Re: [PATCH 1/1] d3dx9: Complete test for D3DXCreateSphere. (try 4)

[Henri Verbeet]

On 26 July 2010 01:20, Misha Koshelev <misha680(a)gmail.com> wrote:
> I kept some macros in, although I know you guys don't like that too much.
>
> Like sphere_vertex. I think it makes the code clearer.
> compare_vertex and compare_face are probably ok, but there's no reason
> sphere_vertex can't be an inline function. Note that macros are much
> trickier to get right than inline functions. E.g. unless you carefully
> control where a macro is used you usually need to at least wrap it
> inside "do {} while (0)", add braces around arguments, and make sure
> arguments are only used once to avoid unintended side-effects.
> +        HeapFree(GetProcessHeap(), 0, (LPVOID)mesh->vertices);
> +    mesh->vertices = (struct vertex *)HeapAlloc(GetProcessHeap(), 0, number_of_vertices * sizeof(struct vertex));

Are you used to programming in C++ by any chance? Assignments to/from
void don't require an explicit cast in C. Also, you can use
"sizeof(*mesh->vertices)" to ensure the type in the HeapAlloc()
matches what you assign it to.
> +struct mesh *new_mesh(DWORD number_of_vertices, DWORD number_of_faces)
> This should be static.
> +    struct mesh* mesh = NULL;
> This is redundant, you assign to mesh a few lines further down.

I know this is mostly a style preference, but *please* just write
"struct mesh *mesh". I guess the idea behind that convention is that
you're declaring "mesh" as a "pointer to struct mesh", but
declarations simply don't work that way in C. I suppose that's a flaw
in the language, but either way it'll break down when you want to do
something like "const char* p, q". In the long term you're probably
better of properly learning C declaration syntax anyway.
> +    /* validate parameters */
> +    assert(number_of_vertices);
> +    assert(number_of_faces);
> You don't need these. There are cases where assert() is appropriate,
> but that's rarely in the tests, and I don't think any of the ones in
> this patch qualify.
> +    /* new mesh */
> +    mesh = (struct mesh *)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct mesh));
> +    if (!mesh)
> +    {
> +        return NULL;
> +    }
> You don't have to allocate "mesh" on the heap, you can just use a
> stack variable and pass a pointer to that.
> +    if (*sine)
> +    {
> +        HeapFree(GetProcessHeap(), 0, (LPVOID)*sine);
> +        *sine = NULL;
> +    }
> Checking for NULL before HeapFree() is redundant.
> +static BOOL compute_circle_table(FLOAT **sine, FLOAT **cosine, FLOAT angle_start, FLOAT angle_step, int n)
> I think a "struct sincos_table" would be nicer. In the rest of the
> code you'd then have have e.g. "theta[stack].sin" instead of
> "sin_theta[stack]", but I think that's ok. You'd also avoid passing
> "FLOAT **sine" and "FLOAT **cosine" to free_circle_table(). Please
> just use "float" instead of "FLOAT".
> +    if (!compute_circle_table(&sin_theta, &cos_theta, theta_start, theta_step, stacks))
> +    {
> +        free_circle_table(&sin_theta, &cos_theta);
> That's redundant again, compute_circle_table() already cleans those up
> itself on failure.