On Sat, Jul 2, 2011 at 1:01 PM, Michael Mc Donnell <michael(a)mcdonnell.dk> wrote:
Thank you for the good review.
Glad you appreciate them. Here is another.
+static HRESULT init_edge_face_map(struct edge_face_map *edge_face_map, CONST DWORD *index_buffer, CONST DWORD *point_reps, CONST DWORD num_faces) +{ ... + TRACE("(%p, %p, %p, %d)\n", edge_face_map, index_buffer, point_reps, num_faces);
A TRACE call for an internal initialization function like this seems unnecessary. They are commonly provided for external functions since it captures application behavior.
+ if (!point_reps) /* Identity point reps */ + { + id_point_reps = generate_identity_point_reps(num_faces, num_vertices); + if (!id_point_reps) + { + hr = E_OUTOFMEMORY; + goto cleanup; ... + hr = iface->lpVtbl->LockIndexBuffer(iface, D3DLOCK_READONLY, &ib_ptr); + if (FAILED(hr)) goto cleanup; ... +cleanup: + HeapFree(GetProcessHeap(), 0, id_point_reps); + if (indices_are_16_bit) HeapFree(GetProcessHeap(), 0, ib); + free_edge_face_map(&edge_face_map); + iface->lpVtbl->UnlockIndexBuffer(iface); + return hr; }
The index buffer is unconditionally unlocked in the cleanup code, but goto cleanup can now be called before the index buffer is locked.
+static DWORD *generate_identity_point_reps(DWORD num_faces, DWORD num_vertices) +{ + DWORD *id_point_reps; + DWORD i; + + TRACE("(%d, %d)\n", num_faces, num_vertices); + + id_point_reps = HeapAlloc(GetProcessHeap(), 0, 3 * num_faces * sizeof(*id_point_reps)); + if (!id_point_reps) + return NULL; + + for (i = 0; i < num_vertices; i++) + { + id_point_reps[i] = i; + } + for (i = num_vertices; i < 3 * num_faces; i++) + { + id_point_reps[i] = -1; + } + + return id_point_reps; +}
Why are there exra point reps allocated with -1 value? Won't only num_vertices point_reps get used?