Comments from reviewers and responses regarding FGT StEvent containers.  Code is at

/star/u/avossen/fgtSwForReview/offline/StFgtDevel/StRoot/StEvent/StFgt*

Justin's comments are in TEAL, Tom's in BLUE.

-Aside from Thomas's comments/suggestions, in general all look reasonable

-Some assert statements here as well to be removed

Done

-For StFgtPoint I'm also curious what this "key" variable will be used for and how it is assigned.

It is a unique identifier, assigned by the pointmaker.  It could have several uses, or may not be used by all code using points.  One possible use is to keep track of which points are associated with which tracks.  Sometimes it better to keep unique number than a pointer to the StFgtPoint class.

One other comment here, probably need a conditional in the constructor to provide an error if one tries to make a point from 2 R strips or 2 phi strips, currently it will just fail the assert.

Yes.  The key is set to -999 and the pointmaker checks the key after constructing.

Also would be good to propogate a position and error for the point similar to the hits (as commented as "TODO").

The TODO comment has been removed.  The pointers to the hits (clusters) are directly accessable, so one can quickly and easily get the position and error for each of the clusters.

StFgtInfo

Notes and suggestions:
• Unnecessary semicolons after ClassDef(), ClassImp() and definition of constructor.

Unnecessary semicolons are being left in, as per comment regarding StFgtA2CMaker.

• virtual destructor and getMessage() may be useful to give the potential for other
classes to inherit from it.

It is extremely unlikely that anyone would inherit from this class.  The plan was to modify this class to hold any needed metadata.  Thus far in commissioning, there has not been a determination if extra metadata needs to be included in this class.  It should either be removed or have extra fields added.  In any case, the two functions have been made virtual.

• #include <TString.h> should move to the header where TString is used.

Done.

StFgtHit

Documentation:
No external documentation provided. Comments with usage information for methods
(preferably Doxygen style) would be useful, as this will likely be heavily used by endusers.

Methods all are returning just a single member field, and the fields all have comments describing what they are.  It is unclear what is missing.  "Endusers", i.e. people analyzing FGT data, will generally deal only with the MuDst containers, not the StEvent containers.  Only those writing tracking algorithms for the StEvent will use this.

Notes and suggestions:
• Default constructor does not initialise mChargeUncert.

Fixed.

• Is a separate default constructor needed? Default values could be provided for the other constructor.

Default values added.

• Will the class be inherited from? Make destructor and methods virtual if this is envisaged.

Extremely unlikely.

• Remove assert() from non-default StFgtHit constructor - handle invalid input some other way (warning messages, setting default values etc).

Done.

• #include <cmath> in StFgtHit.cxx (donʼt rely on something else including it).

Done.

Compilation warnings:
Many extra semicolons (use -pedantic to find them).

See earlier comment regarding extra semicolons.  Also, the "old style casts" have been changed to static_cast

StFgtHitCollection

Move inline definition of Clear() to the header.

The inline is a mistake.  The inline is removed, code remains in the .cxx file

Notes and suggestions:
• If the class basically just wraps StSPtrVecFgtHit then is it needed?

Yes.  It seems to be common procedure to wrap all StSPtrVec's.  I think their is something about requiring inheritance from StObject, which StSPtrVec's do not.  Also, it contains additional information about which disc is it, and

• Where are the points owned? Does StSPtrVecFgtHit take care of deleting them?

This is standard StEvent processing.  Use StSPtrVec's, and somehow, somewhere they are deleted.

• If the collection is copied, can two or more collections potentially reference the same
points? Is this bad? I donʼt know how StSPtrVec... works.

Not necessarily.  It depends on how the StSPtrVec gets copied.  This is in the StCollection files, and it standard STAR code, not anything we're doing different.  We're just following the pattern of other detectors.  It seems to work for all of them, so we're doing the same.

• Why can getNumHits() not just return unsigned int, not size_t?

size_t is unsigned int.

• Make help to make it clear in comments that the collection contains only hits for a
single disk, not the whole FGT.

The comment is added in the description.

• Is the collection intended to be added to by users? If so, protection against adding
hits from the wrong disc may be needed (or not, if only experts use it).

No.  Only the cluster makers add to the collection, which is remaining in expert hands.

Compilation warnings:
Several extra semicolons (use -pedantic to find them).

See earlier comment.

.sl53_gcc432/obj/StRoot/StEvent/StFgtHitCollection.cxx:35: warning: unused parameter 'opt'

Yep.  I'm required to have it by inheritance, but I don't need it.  This warning does not show up under cons and starver eval.

.sl53_gcc432/obj/StRoot/StEvent/StFgtHitCollection.cxx:44: warning: use of old-style cast

Line 44 is "ClassImp(StFgtHitCollection);"  Your pedantic option is too pedantic.