Comments from reviewers and responses regarding StFgtA2CMaker.  The version of the code they looked at is

/star/u/avossen/fgtSwForReview/offline/StFgtDevel/StRoot/StFgtA2CMaker/

Comments from Justin are in TEAL, comments from Tom are in BLUE, my comments in are in black.

-remove printf statements like the following and replace with LOG_DEBUG

#ifdef DEBUG   printf("A2C for iDisc=%d\n",discIdx); #endif

Done, as of Jan. 24th.

-I guess strip fitting over timebins is currently disabled, but will this be the planned mode of operation
(I would think so from all the commissioning working on time bin fits by Len et. al.)?  ie. each strip that
is satisfies some thresholds will be fit over timebins? If so, 2 more questions:

     -how many fits will this be per event in pp500 high luminosity?

     -is the cpu footprint still small relative to total event processing time (maybe its negligible, I don't know)?

Since you have reviewed, much work has been done in understanding the time shape and how to recognize pulses.  A new method has been implemented, which involves just a few criterea.  Fitting every strip was indeed far too time consuming.

-Several commented out "cout" statements should be removed or replaced with LOG_DEBUG -Some lines of code eg: "// if( (mRelThres && adcMinusPed > mRelThres*pedErr) && ((mAbsThres<(-4096)) || adcMinusPed  > mAbsThres) )", are commented out and should either be removed or need a switch added if these are intended to be used for some special cases

Too many cooks in the kitchen on this regard. :)  I've gone through and cleaned it up, removing commented couts and other commented out code.

-General questions: Why is strip->setGeoId( -1 ); the default way to invalidate a strip?  Is there not some better way to flag it as not usable?

Setting the geoid to an invalid number allows one to flag the the invalid state without having to introduce an additional field for this.  The StFgtStripCollection::removeFlagged() function is set to remove all strips with negative geoId.

• The functionality of doCutBadStatus() doesnʼt depend on its argument.

Argument removed

• Forward declare TH1F (donʼt rely on it being included by something else).

Done.

• Unnecessary semicolons after many functions, if and for statements. Compiling with -
pedantic will find them.

This is a little overly pedantic, as the compiler option suggests.  A semi colon after a if{} will not change the functionality nor legebility of the code.  STAR requirements are that it compile with no warnings or errors under the usual settings in cons, not under the option -pedantic.  Semicolons are not being removed.  More important things to worry about.

• mPulseShapePtr and mHistPtr are allocated via new in the constructor but are not
freed. Is this intentional e.g. does ROOT own these objects and delete them itself?

The mPulseShapePtr has been completely removed.  mHistPtr is not deleted in the deconstructor.  Note, all the mHistPtr code has been surrounded by #ifdef MAKE_HISTOGRAM, and may be deleted in the near future.

• mHistPtr->Fit() will print out the fit results - is that needed, or is a quiet fit acceptable?
What happens if the fit does not succeed?

The fits are removed.

• Use StMessMgr instead of printf.

Now use LOG_INFO << Form("...") instead.

Compilation warnings:
Numerous extra semicolons (compile with -pedantic to find them).

Under normal compilation, there are no warnings, only under your extra -pedantic option.  See above comment.

Summary

Noted changes have been checked into latest CVS version.  Need to change keying of DB to elecId instead of geoId, as this is more fundamental.  Also need final decision on whether to remove those parts of the code which are disabled by #ifdef's.  After this, code ready for final handoff.