[Plib-devel] A Third Look Into SSG

[Fay J. F C. AAC/W. <joh...@eg...>]

Gentlemen,

	Part three.

Serious Business:
(1) In "ssgVtxArray.cxx" on line 207 an array of "long" is allocated and its
address stored in a local variable.  I do not see where it is ever deleted
or its address stored somewhere permanent.  This is a memory leak.

Trivial:
(2) More tabs that should be blank spaces in "ssg.h" on lines 643, 676-679,
689, 704, 713, 727 (maybe), 1021, 1422, 1432-1439, 1485, 1577, and 1593; in
"ssgVtxArray.cxx" on line 76 (after the "indices"?)

(3) In "ssg.h", in the definition of "ssg<fill-in>Array", there are a lot of
functions that are defined (with the curly braces, I mean, not just
declared) and are then followed by semicolons.  The semicolons are not
hurting anything, but they are extraneous.  You may find them on lines 487,
488. 490. 492, 509, 510, 512, 514, 531, 532, 534, 536, 553, 554, 556, 558,
576, 577, and 594.

(4) Why is the "type" assignment commented out on lines 607 and 629 of
"ssg.h"?

(5) Lines 684-689 of "ssg.h" appear to be indented improperly.

(6) In the file "ssgTween.cxx", the construction "banked_vertices ->
getNumEntities ()" is used on lines 128, 168, 246, and 450.  On line 221 the
function "getNumBanks ()" is used; I consider the latter to be clearer for
the purpose and a little investigation shows it to be identical to the
former.  I suggest that the former be changed to the latter.

(7) In the file "ssgTween.cxx" on lines 150, 184, 192, 200, and 208, the
variable "bsphere_is_invalid" is set to true immediately after a call to
"dirtyBSphere".  The call includes that assignment and so the repeated
assignment is not necessary.

(8) In the file "ssgVtxArray.cxx" on lines 354, 370, and 392 we have
"assert(false)" statements to kill execution.  May I suggest a call to
"ulError ( UL_FATAL, <error_message> )" instead?

Not so trivial:
(9) Do we want to define a template class for the "ssg<fill-in>Array"
classes?

(10) On line 736 of "ssg.h" we see that a "setHandle()" function is
deprecated.  Should we remove it, or at least give a date of deprecation so
that people will know how quickly they should move away from it?

(11) In the file "ssgVtxArray.h", on line 207 (site of the memory leak
mentioned earlier), the variable "oldIndex2NewIndex" is defined to be a
pointer to an array of longs.  Then on line 228 the value is "static_cast"
to short.  Why not just declare the array to be of type short in the first
place?

John F. Fay
joh...@eg...
850-729-6330