make file var shape_ const since it is only read

[gartung]

File vars need to be const qualified when possible for thread safety. Once instantiated this never gets modified in production.

[cmsbuild]

A new Pull Request was created by @gartung (Patrick Gartung) for CMSSW_7_1_X.

make file var shape_ const since it is only read

It involves the following packages:

CondFormats/CastorObjects

@apfeiffer1, @diguida, @cmsbuild, @nclopezo, @rcastello, @ggovi, @Degano can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[cmsbuild]

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2413/127/summary.html

[apfeiffer1]

+1

[ktf]

Bypassing AlCa signature. Complain if not ok. The day we will allow constexpr constructors in our data formats is always too far away.

[diguida]

+1
Ok for us, no complains at all when thread safety is involved!

[VinInn]

ahhhgggg
this is anonymous namespace in a header file!!!!
is the same as C static (is our so smart static analyzer not able to catch anonymous namespace in header files?)
it will be local and hidden in each file where the header is included....
Please either move in the compilation unit were is used or make it class static if needed in more than one compilation unit!

[gartung]

It has global storage so it is flagged per @Dr15Jones requirements:

https://cmssdt.cern.ch/SDT/lxr/source/Utilities/StaticAnalyzers/src/FunctionChecker.cpp?v=CMSSW_7_1_X_2014-02-14-0200#133

[VinInn]

I am not aging about thread safety,
I am arguing about the fact that it will not work as intended if accessed from multiple compilation units even in the same library. It's a bug
nm -C $CMSSW_RELEASE_BASE/lib/slc6_amd64_gcc481/libCondFormatsCastorObjects.so | grep shape_
000000000007a040 b (anonymous namespace)::shape_
000000000007a280 b (anonymous namespace)::shape_
000000000007a4e0 b (anonymous namespace)::shape_

[diguida]

Hi!
This could easily be a static const in the class definition: it is only accessed by

const CastorQIEShape& getShape () const

and the class interface does not modify it.
@VinInn am I right?

[VinInn]

yes, I think you are right.

[gartung]

Making shape_ static const class member gives compilation errors.
Making shape_ a function static const var works:

const CastorQIEShape& getShape () const { return shape_;}
const CastorQIEShape& getShape () const { static const CastorQIEShape shape_;return shape_;}

nm -C ~/CMSSW_7_1_0_pre2/lib/slc5_amd64_gcc481/libCondFormatsCastorObjects.so | grep shape_
0000000000079f40 V guard variable for CastorQIEData::getShape() const::shape_
0000000000079d30 V CastorQIEData::getShape() const::shape_

[gartung]

OK, Chris showed me how to initialize a static const member correctly in the .cc file and nm shows only one symbol:

nm -C ~/CMSSW_7_1_0_pre2/lib/slc5_amd64_gcc481/libCondFormatsCastorObjects.so | grep shape_
0000000000079d60 B CastorQIEData::shape_

I will submit a pull request for this change.

[VinInn]

and no "guard variable" (used to make the function static construction thread safe)
excellent!

[gartung]

#2473

[diguida]

Thanks!