On Sat, Jul 2, 2011 at 1:01 PM, Michael Mc Donnell michael@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?