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

Henri Verbeet hverbeet at gmail.com
Mon Jul 26 08:49:44 CDT 2010


On 26 July 2010 01:20, Misha Koshelev <misha680 at 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.



More information about the wine-devel mailing list