Misc ICC errors fixes.

[ktf]

• ICC complains because we are trying to create a std::pair<int, int> from what is actually a (int, size_t) pair. This fix will workaround the problem by forcing a downcast of size_t to int, but it would result in a real issue if secondLegNr was > 2**32. However, I assume we would have other problems in that case.
• Like clang, ICC does not have constexpr version for std::exp / std::pow and in general anything coming from cmath. I think this is actually more an unsupported gcc extension.

[cmsbuild]

A new Pull Request was created by @ktf (Giulio Eulisse) for CMSSW_7_4_X.

Workaround ICC error.

It involves the following packages:

HLTrigger/Egamma

@Martin-Grunewald, @perrotta, @cmsbuild, @nclopezo, @fwyzard can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.

[cmsbuild]

Pull request #7406 was updated. @perrotta, @cmsbuild, @nclopezo, @cvuosalo, @fwyzard, @Martin-Grunewald, @slava77 can you please check and sign again.

[Martin-Grunewald]

This gives a compiler warning:

/scratch/CMS2/CMSSW_7_4_X_2015-01-28-0200/src/RecoLocalCalo/EcalRecAlgos/src/ESRecHitSimAlgo.cc:54:20: warning: extra tokens at end of #ifdef directive
   #ifdef __clang__ || __INTEL_COMPILER
                    ^

[ktf]

@cmsbuild please test

[cmsbuild]

Pull request #7406 was updated. @perrotta, @cmsbuild, @nclopezo, @cvuosalo, @fwyzard, @Martin-Grunewald, @slava77 can you please check and sign again.

[davidlt]

A few comments.

• ICC and Clang are correct with cmath functions. Recent changes in C++14 standard prohibits implementations mark random standard library functions as constexpr. IIRC, GCC will revert those back to non-constexpr. They made them as constexpr before the standard was finished.
• I would avoid static_cast<>. This is not fixing and issue, but hiding it. matchedCands seems to store size_t, yet matched2ndLegs a pair of ints? They size_t is not a good type here? On 64-bit systems it would be long unsigned int and on 32-bit - unsigned int. Someone needs to pick correct size types and be consistent.

[fwyzard]

Hi David,
since you mention C++14 - do we plan on switching to -std=c++14 for CMSSW ?

.A

[ghost]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.

[ktf]

@davidlt

• As of today gcc digests constexpr. When they change it or we move to c++14, we think about it. No change of behaviour in production builds due to platform ports.
• matchedCands is std::vector<std::pair<int,int> > matchedCands; so it stores int. changing it to store size_t changes memory footprint. Again, no changes in behaviour in ports. static_cast is simply making the implicit explicit.

[davidlt]

Regarding 2nd point. From my experience with ARMv7/UBSan/etc/you need to be very careful with types. I think, C++ 4.7/3 applies here (integral conversion):

If the destination type is signed, the value does not change 
if the source integer can be represented in the destination type. 
Otherwise the result is implementation-defined. (Note that this 
is different from signed integer arithmetic overflow, which is 
undefined) 

Now you have it easy. Later on if this becomes a problem you wont have a compiler diagnostic, you will have a difference in histogram, which will be way harder to track.

My two cents. It's HLT and RECO decision.

@fwyzard I think, this was discussed in Core Software meeting and it was OKed to have try -std=c++14 with CMSSW (maybe as separate IB?), but that's there it stopped.

[ktf]

Changing secondLegNr is simply moving the problem around, since std::vector<T>::size() always returns size_t which would then need to be casted. The only real solution without having 50% more memory used in that vector is to have an assert so that the size_t is within int range and do then do a static cast. There is no way that variable is going to exceed 2**31 nor become negative.

[cmsbuild]

+1
Tested at: f762ec0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7406/2176/summary.html

[slava77]

regarding
As you say this is RECO / HLT business, if changing from int to size_t is fine with them
we should not make this change in data formats in the persisted data members (IIUC, it can trigger a difference in the actual compiled dictionary depending on the architecture).
Beyond that, in memory-lght cases size_t should be fine, do a cast otherwise.

[ktf]

Ok, I think we all agree that if this needs to be fixed, it can be done in a separate PR. @slava77, can you +1 this? It should be trivial.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7406/2176/summary.html

[ktf]

Assuming @slava77 is ok with the changes, which are otherwise trivial. Merging. Complain if not.

[slava77]

+1

for
#7406 f762ec0
postfactum
code looks ok
the set of tests in jenkins is surprisingly short