[LTO_X - Alignment & Calibration] Solve -Wstrict-overflow compiler warnings #38649
Conversation
aandvalenzuela
changed the title
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38649/30934
|
|
A new Pull Request was created by @aandvalenzuela (Andrea Valenzuela) for master. It involves the following packages:
@malbouis, @yuanchao, @cmsbuild, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild , please test |
|
@cmsbuild , please test for CMSSW_12_5_LTO_X |
|
type trk,bugfix |
| @@ -383,7 +383,7 @@ void OverlapValidation::analyzeTrajectory(const Trajectory& trajectory, | |||
| ++overlapCounts_[1]; | |||
| if ((layer != -1) && (acceptLayer[subDet])) { | |||
| for (vector<TrajectoryMeasurement>::const_iterator itmCompare = itm - 1; | |||
| itmCompare >= measurements.begin() && itmCompare > itm - 4; | |||
| itmCompare >= measurements.begin() && itmCompare - (itm - 4) > 0; | |||
There was a problem hiding this comment.
wouldn't
itmCompare > (itm - 4);work as well and be slightly more readable?
There was a problem hiding this comment.
For me it was also more readable, but I still get the warnings in this form:
>> Package Alignment/TrackerAlignment built
/build/tmp/CMSSW_12_5_LTO_X_2022-07-07-1100/src/Alignment/OfflineValidation/plugins/OverlapValidation.cc: In member function 'analyzeTrajectory':
/build/tmp/CMSSW_12_5_LTO_X_2022-07-07-1100/src/Alignment/OfflineValidation/plugins/OverlapValidation.cc:386:47: warning: assuming pointer wraparound does not occur when comparing P +- C1 with P +- C2 [-Wstrict-overflow]
386 | itmCompare >= measurements.begin() && itmCompare > (itm - 4);
| ^
Leaving library rule at src/Alignment/OfflineValidation/plugins
There was a problem hiding this comment.
for my education how do you compile with this flag?
There was a problem hiding this comment.
just use CMSSW_12_5_LTO_X IBs
There was a problem hiding this comment.
how about
(itmCompare >= measurements.begin()) && ((itmCompare > (itm - 4)) > 0);
There was a problem hiding this comment.
This also works :)
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-318ed6/26076/summary.html Comparison SummarySummary:
|
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-318ed6/26075/summary.html Comparison SummarySummary:
|
|
There is a warning of the same type in I would also propose something similar as for Any other proposal in this case? |
|
+alca
|
|
please abort |
| @@ -383,7 +383,7 @@ void OverlapValidation::analyzeTrajectory(const Trajectory& trajectory, | |||
| ++overlapCounts_[1]; | |||
| if ((layer != -1) && (acceptLayer[subDet])) { | |||
| for (vector<TrajectoryMeasurement>::const_iterator itmCompare = itm - 1; | |||
| itmCompare >= measurements.begin() && itmCompare > itm - 4; | |||
| (itmCompare >= measurements.begin()) && ((itmCompare > (itm - 4)) > 0); | |||
There was a problem hiding this comment.
@aandvalenzuela This should also be changed:
((itmCompare > (itm - 4)) > 0)
Solve -Wstrict-overflow warnings Solve -Wstrict-overflow warnings Solve -Wstrict-overflow warnings Fix signed overflow warning Apply smuzaffar suggestion Fix style Apply smuzaffar suggestion Apply correction to initial suggestion
aandvalenzuela
force-pushed
the
from-CMSSW_12_5_LTO_X_2022-07-07-1100-Alignment
branch
from
096d968
to
805069f
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38649/31044
|
|
Pull request #38649 was updated. @malbouis, @yuanchao, @cmsbuild, @francescobrivio, @ChrisMisan, @tvami can you please check and sign again. |
|
please test |
|
please test for CMSSW_12_5_LTO_X |
|
please take also in account the discussion going on at #38683 (comment) |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-318ed6/26234/summary.html Comparison SummarySummary:
|
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-318ed6/26233/summary.html Comparison SummarySummary:
|
|
-alca
|
|
hold |
|
Pull request has been put on hold by @perrotta |
| @@ -383,7 +383,7 @@ void OverlapValidation::analyzeTrajectory(const Trajectory& trajectory, | |||
| ++overlapCounts_[1]; | |||
| if ((layer != -1) && (acceptLayer[subDet])) { | |||
| for (vector<TrajectoryMeasurement>::const_iterator itmCompare = itm - 1; | |||
| itmCompare >= measurements.begin() && itmCompare > itm - 4; | |||
| (itmCompare >= measurements.begin()) && ((itmCompare + 4) > itm); | |||
There was a problem hiding this comment.
I'd argue comparing an iterator to be larger than or equal with begin() does not make much sense as in principle it is always true because
The begin iterator is not decrementable and the behavior is undefined if
--container.begin()is evaluated.
https://en.cppreference.com/w/cpp/named_req/BidirectionalIterator
There was a problem hiding this comment.
but itm-1 has a similar issue when itm is equal to measurements.begin(), no? or is that somehow ok?
There was a problem hiding this comment.
but itm-1 has a similar issue when itm is equal to measurements.begin(), no?
Right (didn't look that far), already the itm-1 seems to be potentially undefined behavior.
How about rewriting the loop e.g. along
for (int offset = 1, end=std::min(3, std::distance(measurements.begin(), itm)); offset <= end; ++offset) {
auto itmCompare = std::prev(itm, offset);
...
}Then, for
itm == begin()the loop body would not be runitm == begin()+1the loop body would be run withitmCompare = itm-1itm == begin()+2the loop body would be run withitmCompare = itm-1anditm-2itm >= begin()+3the loop body would be run withitmCompare = itm-1,itm-2, anditm-3
(I'm sure there are even better ways to write the loop)
|
|
Hello,
We have seen some compiler warnings of the type
-Wstrict-overflowin LTO_X IBs (CMSSW_12_5_LTO_X_2022-07-07-1100 and CMSSW_12_5_LTO_X_2022-07-06-1100, for example) in bothAlignment/OfflineValidationandAlignment/TrackerAlignmentpackages. See sample stack traces, respectively:and
Regarding the type of error and following some online discussions of this type of warning fmtlib/fmt/issues/2757, I have tried to workaround it as shown in this PR, but feel free to propose other solutions that could be better regarding the analysis itself.
Many thanks,
Andrea.