Home » Groups » STAR Protected

StFgtDbMaker/StFgtGeom Review

Updated on Sun, 2012-03-18 20:54. Originally created by rfatemi on 2012-02-09 09:54.

 Summary of Reviewer's Comments

RESPONSE TO EVALUATION by Justin Stevens

StFgtDbMaker

  • StFgtDbIdealImp removed and StFgtDbImpl collapsed into StFgtDb
  • asserts removed
  • constants removed
  • beginTime statements included in StFgtDbMaker when tables are loaded

StFgtGeom

  • See comment to Burton below - we are under impression that all static numbers (widths of strips, location within disc etc) should be hardcoded and not put into database.

 

RESPONSE TO  EVALUATION of StFgtGeom by Tom Burton

CONSTRUCTOR: Changed comment about instances of StFgtGeom.  Class is still singleton even with multiple instances because instances do not have their own data.
MEHOD NAMES: Updates method names to conform to typical camel casing
FIELD NAMES: changed 2pi, pi, halfpi to confom.  The standards are unclear about a struct acting as a class or a data member.  We would prefer not to make all of these name changes to the code.
PRIVATE/PUBLIC/PROTECTED : We agree - fixed.
INLINING: We agree and moved some of the longer inlines to .cxx.
DOCUMENTATION: done

NOTES & SUGGESTIONS:

  • During DB discussions with S&C we were instructed to have all non-dynamic geometries hard-coded.  Only parameters that change with time should be in the database. We would consider changing this if requested/approved by Jerome+Dmitry.  
  • Agree - made StFgtGeomData a nested class of StFgtGeom::Data
  • The StFgtDbFileMaker is an expert-only code that lives in StFgtPool and is used by experts to make database mapping files.  It needs to access private members in StFgtGeom and this is why it is a friend.  It will not be reviewed.
  • ROOT::TMath used for all pi variables.
  • Boundry checks are implemented
  • see above
  • All asserts removed.
  • see boundry checking issue above
  • removed from header and .cxx

WARNINGS: ( cons EXTRA_CXXFLAGS="-Wextra -pedantic -ansi -Winit-self -Wold-style-cast -Woverloaded-virtual -Wuninitialized -Wmissing-declarations -Winit-self")

1) FIXED - ./StRoot/StFgtUtil/StFgtConsts.h:39: error: comma at end of enumerator list
In file included from .sl53_gcc432/obj/StRoot/StFgtUtil/geometry/
StFgtGeom.cxx:9:

2) RDO+ARM are passed to getNaiveMapping to have a consistant interface across classes.  The StFgtDb class contains a getMapping which does use RDO and ARM and therefore we would prefer to pass it as well to the getNaiveMapping in StFgtGeom.h   This also allows RDO+ARM to be used in the future NaiveMappings.

.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.h:327: warning: unused
parameter 'rdo'

.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.h:327: warning: unused
parameter 'arm'

3) Same explanation for isNaiveR as for getNaiveMapping in 2)

.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.h:339: warning: unused
parameter 'rdo'

.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.h:339: warning: unused
parameter 'arm'

4) FIXED - .sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx:67: error: extra ';'

5) pbins was changed to const but still problem - not at all clear why.

.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx: In static member
function 'static int StFgtGeom::phi2LocalStripId(double, double, double*)':
.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx:129: error: ISO C++
forbids variable length array 'pstrip'

6) binFrac may be used in future - would prefer to leave for now.

.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx: At global scope:
.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx:124: warning: unused
parameter 'binFrac'

7) rstrip was changed to const but still problem - not clear why

.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx: In static member
function 'static int StFgtGeom::rad2LocalStripId(double, double, double*)':
.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx:630: error: ISO C++
forbids variable length array 'rstrip'

8) see comment 6) .sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx: At global scope:
.sl53_gcc432/obj/StRoot/StFgtUtil/geometry/StFgtGeom.cxx:625: warning: unused
parameter 'binFrac'

 

RESPONSE TO EVALUATION of StFgtDbMaker  by Tom Burton

 
ASSIGNMENT OPERATOR:  omission is implied as intentional
COPY CONSTRUCTOR: ommision is implied as intentional
METHOD NAME: changed to setFlavor
INLINING:  This is a stylistic preference
DOCUMENTATION: done
  • removed
  • private moved
  • removed gStFgtDbMaker
  • indentation fixed
  • Yes - it is needed for user work flow purposes
  • moved
  • removed asserts
 
RESPONSE TO EVALUATION of StFgtDbIdealImpl  by Tom Burton
 
This class was removed
 

RESPONSE TO EVALUATION of StFgtDbImpl  by Tom Burton

*NOTE:  When StFgtDbIdealImpl was removed StFgtDbImpl was collapsed into StFgtDb.  But changes below were preserved in StFgtDb.
 
DESTRUCTOR: added virtual destructor
FIELD NAMES: Coding guidelines say each class data member needs to start with an "m".  No restrictions are placed on the case of the following character.
INLINING: We did this for StFgtGeom and it took too much time.  It is just a stylistic concern so we are leaving as written
  • We can't currently restrict access because we may have more than one factory class. We could use friend functions for every factory class, but we're also supposed to avoid the use of friend classes, as per the coding standard.
  • all pointer fields have been initialized to NULL
  • Will include checks for NULL pointers in boundry checks implemented (see StFgtGeom)
  • Will use returns for error return
  • asserts removed
  • remains the same

 

RESPONSE TO EVALUATION of StFgtDb  by Tom Burton 

CONSTRUCTOR: comment states this is abstract base class which by definition has no constructor/assignment or copy
DESTRUCTOR: added virtual destructor
ASSIGNMENT: definition of abstract base class
COPY:  definition of abstract base class
METHOD : removed badly named functions
INLINING: done
DOCUMENTATION:  done
 
NOTES & SUGGESTIONS:
  • we prefer getXXXfromYYY naming convention so that function is explicit in name
  • we think this comes down to preference and chose to leave function as written
  • gridAttenuation removed
  • printFgtDumpCVS1 - assert removed, fstream used, *this is only thing that works (StFgtDb::function doesn't), remains virtual to avoid runtime error
  • extraneous includes in header removed

 

Groups: