Re: [PATCH 2/7] d3dx9_36/tests: Added HLSL test suite

[Henri Verbeet]

On 25 August 2010 09:58, Travis Athougies <iammisc(a)gmail.com> wrote:
> +static IDirect3DPixelShader9* compile_pixel_shader(IDirect3DDevice9* device, const char *shader, const char *profile,·
> +»······»·······»·······»·······»·······»·······   ID3DXConstantTable **constants)
> You still have basic formatting errors like trailing spaces, mixing
> tabs and spaces, inconsistent "*" placement, etc.
> +    hres = IDirect3DDevice9_CreateVertexShader(device_ptr, (DWORD*) ID3DXBuffer_GetBufferPointer(compiled), &vshader_passthru);
> Here's another unnecessary cast. For the record, I'm not going to
> point out every single flaw in every single patch. If I find an issue
> in a patch, you should take that as a hint that perhaps you didn't
> review your patches careful enough before sending them, and go over
> them again yourself.

Anyway, we've decided to implement the shader assembler and compiler
in the d3dcompiler dll. I think it makes sense to move any
compiler/assembler tests there as well. Note that I'm going to request
that d3dcompiler follows the same general style as used in
dxgi/d3d10core/d3d10, this is different from the style used in most of
d3dx9.

Something else. In my opinion an internship should imply some level of
guidance/training, not simply "cheap labor". In particular, while
ideally you would have caught the issues mentioned in this and
previous reviews yourself, whoever has been mentoring you certainly
should have, these are basic C / Wine development issues. The process
is supposed to look somewhat like this:

1. Carefully write the patch.
2. Carefully review the patch.
3. If you find any issues, carefully fix them and go back to 2.
4. Run the test suite.
5. If it finds any issues, carefully fix them and go back to 2.
6. Send the patch to your mentor for review.
7. If he finds any issues, carefully fix them and go back to 2.
8. If it's still the same day as on 1, consider waiting a day, so you
can give the patch a fresh look in the next step.
9. Repeat steps 2 through 5 before sending the patch to wine-patches.
10. Send the patch to wine-patches.
11. If it finds any issues, carefully fix them and go back to 2.

Replace "patch" with "patchset" if you're doing more complicated
things. Ideally step 6 would produce something like a "Signed-off-by"
or "Reviewed-by" tag.

If you're at step 11 for the third or fourth time, and there are still
problems with the patch, there are structural failures at 1, 2 and 6.
Failures at 1 and 2 are somewhat expected for an internship, depending
on what kind of standards a specific organization sets, but that's
exactly what step 6 is supposed to catch. I.e., when I find an issue
in a patch that means at the very least that you didn't catch it when
writing the patch, didn't catch it when reviewing the patch the first
time, your mentor didn't catch it, and you didn't catch it during
review before sending the patch. The things wine-patches reviews are
supposed to catch are non-trivial interactions with other parts of the
code, not trivial whitespace errors.