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.