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.

---

StFgtPoint

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.

It is unknown how much this container wil be used until after tracking issues are decided, many monthes after this review.  And all methods are just simply returning a single value, which is obvious from the name.

Notes and suggestions:
• Two constructors could be replaced with a single constructor with default values.

No.  The no-argument contructor sets the hit pointers to zero, while the other constructor displays an error if the user supplies pointers that are zero.   Combining the two would result in the error that the user passed null pointers, when really the no-argument constructor was used.

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

No, it will not.

• What is ʻkeyʼ? Indicate in comments.

It is already in the comments.  See the line

   Int_t mKey;                         // unique label

The integer key is a unique label to identify the point.

• Remove assert() from constructor, handle bad input in another way.

Done.

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

These are staying in, as per earlier comments.

Use of old style cast

Fixed

---

StFgtPointCollection

Documentation:
No external documentation provided. Comments are probably sufficient.
Notes and suggestions:
• If the class basically just wraps StSPtrVecFgtPoint then is it needed?

Because standard STAR practice is to wrap all StSPtrVec's--at least this was the case for all the examples I looked at.  The code is also much cleaner to have a static array of StFgtPointCollections rather than a static array of StSPtrVecFgtPoints.

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

Yes.  This is how StSPtrVec's worked, as defined in StArray.h.  Not our code--just following standard practise.

• 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.

Already ansered before--it's fine.

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

Answered before, size_t is unsigned int.

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

They are being left in, as per earlier comments.

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

See earlier comment about unused parameter 'opt'

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

This is part of 'ClassImp(StFgtPointCollection);'  ROOT uses old-style casts, as does other STAR code.  Why do you care so much about them?

---

StFgtStrip

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.

Again, most StEvent containers generally only used by experts.  All methods are just simply returning the values which are obvious from their name.

Notes and suggestions:
• kInvalidChargeValue: an anonymous enum with only one enumerated value might as
well just be a class constant. If it is kept, move protected section to after public.

And on the same token, a class constant might as well be a single anonymous enum.  The anonymous enum is nice, as it is cleaner to write the value with the declaration, while a const member function would be decleared it in the .h and defining the value in the .cxx.   It has been combined with the other protected, rather than coming before the public section.

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

No, this is extremely unlikely.

• There is a comment after declaring mAdc about DAQ_FGT using unsigned, not signed, short. Should these not use the same?

No.  They do not have to be the same.  And after pedestal subtraction, mAdc can in fact be negative, so we need the sign.

• std::vector is preferable to array for mAdc. Also, an explicit copy constructor and assignment operator would not be needed then.

This is not neccessarily true.  std::vector would introduce a lot of overhead, which is unneeded because the length is fixed.  Many, many static arrays are prevelent in other StEvent containers for other detectors.  Also, the current STAR version of cons does not support std::vectors well when writing to disc.   The only cost of using a static array was writing the explicit copy and assignment operator.  But this took very few minutes, and it is done now.   There is no disadvantage to the static array, and many advantages.

• Any issue with self-assignment in assignment operator? If such a test is deliberately ignored, I would at least put a comment to that effect. Also, call StObject::operator=here.

No issue--comment added.  Also, no need to call StObject::operator=, since StObject has no fields to copy other than those inherited from TObject, which are in any case not used by StFgtStrip.  Other detectors also omit the StObject::operator=.  See, e.g., http://www.star.bnl.gov/cgi-bin/protected/cvsweb.cgi/StRoot/St_base/StObject.h?rev=1.17

Compilation warnings:
Several extra semicolons (use -pedantic to find them).
.sl53_gcc432/obj/StRoot/StEvent/StFgtStrip.cxx:100: warning: use of old-style
cast

Again, semicolons are staying.  The old-style cast is in the ClassImp.

---

StFgtStripCollection

Inlining: Move inline definition of Clear() to the header.

Inlining was a mistake--inline keyword removed.

Documentation:
No external documentation provided. Comments are sufficient.
Notes and suggestions:
• 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.
• Why can getNumStrips() not just return unsigned int, not size_t?

All these have been answered before.  Same answers apply to this class.

• Consider using mStripElecIdVec.at(elecId) rather than mStripElecIdVec[elecId].

We'd rather not waste the overhead.  As long as one always uses the standard functions to determine an elecId, this will be fine.  Again, these containers are generally only used by experts.

• Would be nice to have a comment in getStrip() indicating what takes ownership of the new StFgtStrip().

Done.

• Move #include <vector> to header (donʼt rely on something else to include it).

Done.

Compilation warnings:
Several extra semicolons (use -pedantic to find them).
.sl53_gcc432/obj/StRoot/StEvent/StFgtStripCollection.cxx:91: warning: unused parameter 'opt'
.sl53_gcc432/obj/StRoot/StEvent/StFgtStripCollection.cxx:112: warning: use of old-style cast

Again, semicolons are staying.  The old-style cast is in the ClassImp.

---

StFgtTrackCollection

Inlining: Move inline definition of Clear() to the header.

Inline was mistake--keyword removed.

Documentation:
No external documentation provided. Comments are sufficient.
Notes and suggestions:
• 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.
• If the class basically just wraps StSPtrVecFgtTrack then is it needed?
• Why can getNumTracks() not just return unsigned int, not size_t?

Compilation warnings:
Several extra semicolons (use -pedantic to find them).
.sl53_gcc432/obj/StRoot/StEvent/StFgtTrackCollection.cxx:34: warning: unused parameter 'opt'
.sl53_gcc432/obj/StRoot/StEvent/StFgtTrackCollection.cxx:45: warning: use of old-style cast

All of these comments have aready been answered for other classes.  Same answers apply for this class as for the others.

---

StFgtCollection

Documentation:
No external documentation provided. Comments are sufficient.
Notes and suggestions:
• Consider replacing arrays with vectors, [] with at().
• Why specifically return size_t instead of unsigned int (they should be the same anyway).
Compilation warnings:
Several extra semicolons (use -pedantic to find them).
.sl53_gcc432/obj/StRoot/StEvent/StFgtCollection.cxx:60: warning: use of oldstyle cast

All of these comments have aready been answered for other classes.  Same answers apply for this class as for the others.

• If this is not an expert-only class, make it clear with a comment that the user should not attempt to delete pointers returned by get...() methods.

It is basically an expert only class, but I've added a comment in any case.

• Why is StMuDstMaker a friend (not necessarily an objection, but no comment to explain why).

Comment added.  It's needed for StMuDstMaker, to copy the data to MuDst.  This is common amoung other makers.