On Mon, Aug 22, 2016 at 12:44 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
I have a few pretty generic suggestions. You don't need to include configure changes in the patch, they will be regenerated by Alexandre anyway since those depend on the version of the autotools.
I was thinking of this, so looked at some other patches which changed configure.ac, which also appeared to alter configure script, so did this. Will remove that change, it's cleaner this way indeed.
Did you look into adding those tests to dxdiag instead of as a separate program? I don't think a separate program is necessarily bad but if integrating into dxdiag is reasonable it's probably better to do that from the start.
Yes, I've even made a dialog for dxdiag with the System tab, but when I began working on the Display tab, it appeared to require some functionality which Wine's dxdiag.dll doesn't even support — providing information about video card, its memory etc. I decided it'd be ugly to just show several buttons to launch demos with no other information, so did the demos a separate program. In the future it'll be not that hard to integrate the demo as just a button triggering CreateProcess(). The demos do return specific error codes on each error, so the dxdiag UI would be able to record more or less explicit log of why demo failed. Also, I think it'd be better to make the demos runnable even if for some strange reason dxdiag framework were broken, so this was another reason why I decided to make it a separate program.
Avoid camelCase or similar mixed-case identifier styles, just use lowercase_with_underscores. For pointer declarations you want to stick the '*' to the variable, not the type. Also please avoid LPTYPES, just use explicit pointer types. C++ comments (// comment) aren't allowed in Wine sources. Please replace them with normal C comments (/* comment */), or drop them entirely if they don't add anything that isn't clear from just looking at the code.
Oh yeah, I was trying to avoid C99 comments, but as GCC doesn't seem to have a switch to warn about using them, some did slip through.
It would be better to use explicit float constants when assigning to float variables (e.g. D3DMATERIAL).
There aren't many other general style rules in Wine (it's mostly about using the style already used in the component so for new components you're mostly free to pick your own) as long as there is consistency. I see you mixing space after ',' with no space, pick one (preferably the space variant IMO) and stick to it. We usually use parentheses with the "sizeof" operator in Wine sources, so while it's not strictly required it would probably be nice to follow the general trend.
More specific comments: I'd split this over multiple patches. You can probably make an initial patch adding the program with no real functionality, then multiple patches adding the various graphical API demos. In that regard, the d3d8 / d3d9 "merged" handling via macros seems a bit obfuscating to me. Just replicating the code twice seems cleaner. Maybe you can create a few helper functions for the common code.
D3D code for 8 and 9 being merged was an attempt to avoid forgetting to fix one while fixing another. D3D 8&9 are too similar to need separate files. But OK, I'll split it then.
BTW, you can probably make things a bit more structured by moving the main loop to common code and have it call API backend functions. You'd have for each backend a structure like:
static const struct render_backend d3d9_backend { d3d9_list_modes, d3d9_init, d3d9_draw, d3d9_wndproc, d3d9_destroy, };
(just an idea WRT the actual functions, I haven't really looked in detail) and you'd pass the structure from the selected API to the "main loop" function.
I don't think you're using GLU anymore (good) so you should drop the glu32 import.
Yeah, just forgot about the import.