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