-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140
[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34140/23336
|
A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master. It involves the following packages: CommonTools/RecoAlgos @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: Build BuildI found compilation error when building: >> Compiling /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_0_X_2021-06-15-1100/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t.cpp >> Building binary ErrorPropOri_t /cvmfs/cms-ib.cern.ch/nweek-02685/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t/ErrorPropOri_t.cpp.o: in function `main': ErrorPropOri_t.cpp:(.text.startup+0x7b6): undefined reference to `AnalyticalCurvilinearJacobian::computeFullJacobian(GlobalTrajectoryParameters const&, Point3DBase const&, Vector3DBase const&, Vector3DBase const&, double const&)' /cvmfs/cms-ib.cern.ch/nweek-02685/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: ErrorPropOri_t.cpp:(.text.startup+0xba0): undefined reference to `AnalyticalCurvilinearJacobian::computeFullJacobian(GlobalTrajectoryParameters const&, Point3DBase const&, Vector3DBase const&, Vector3DBase const&, double const&)' collect2: error: ld returned 1 exit status >> Deleted: tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t/ErrorPropOri_t gmake: *** [tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorPropOri_t/ErrorPropOri_t] Error 1 >> Compiling /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_0_X_2021-06-15-1100/src/TrackingTools/AnalyticalJacobians/test/ErrorProp_t.cpp >> Building binary ErrorProp_t /cvmfs/cms-ib.cern.ch/nweek-02685/slc7_amd64_gcc900/external/gcc/9.3.0/bin/../lib/gcc/x86_64-unknown-linux-gnu/9.3.0/../../../../x86_64-unknown-linux-gnu/bin/ld: tmp/slc7_amd64_gcc900/src/TrackingTools/AnalyticalJacobians/test/ErrorProp_t/ErrorProp_t.cpp.o: in function `main': |
const GlobalVector& p, | ||
const GlobalVector& h, | ||
const double& s) { | ||
inline void AnalyticalCurvilinearJacobian::computeFullJacobian(const GlobalTrajectoryParameters& globalParameters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@makortel , looks like this change caused the build error #34140 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I woiuld suggest to add //NOLINT(misc-definitions-in-headers) for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. In this case the AnalyticalCurvilinearJacobianSSE.icc
(and AnalyticalCurvilinearJacobianEXT.icc
) are more source-like than header-like files. An easy way would be to rename these files to use another file extension, but that was disfavored earlier (#33366 (comment)).
An alternative would be to rename those files to .cc
, and move the #ifdef
s there along
// in AnalyticalCurvilinearJacobian.cc
#if !(defined(USE_SSEVECT) || defined(USE_EXTVECT)) || defined(TRPRFN_SCALAR)
void AnalyticalCurvilinearJacobian::computeFullJacobian(...) { ... }
#endif
// in AnalyticalCurvilinearJacobianSSE.cc
#if defined(USE_SSEVECT) && !defined(TRPRFN_SCALAR)
void AnalyticalCurvilinearJacobian::computeFullJacobian(...) { ... }
#endif
// in AnalyticalCurvilinearJacobianEXT.cc
#if defined(USE_EXTVECT) && !defined(TRPRFN_SCALAR)
void AnalyticalCurvilinearJacobian::computeFullJacobian(...) { ... }
#endif
but keeping the #else
case manually up to date would be prone to errors.
So maybe the NOLINT
would be least bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @makortel , I have added // NOLINTNEXTLINE(misc-definitions-in-headers)
for this
@@ -6,7 +6,7 @@ template <> | |||
#ifdef __CUDACC__ | |||
void CAHitNtupletGeneratorKernelsGPU::allocateOnGPU(int32_t nHits, cudaStream_t stream) { | |||
#else | |||
void CAHitNtupletGeneratorKernelsCPU::allocateOnGPU(int32_t nHits, cudaStream_t stream) { | |||
inline void CAHitNtupletGeneratorKernelsCPU::allocateOnGPU(int32_t nHits, cudaStream_t stream) { | |||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, should we add the following for both of these definitions?
// NOLINTNEXTLINE(misc-definitions-in-headers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we could do the same trick as in #33366 (comment), i.e. rename CAHitNtupletGeneratorKernelsAlloc.h
to CAHitNtupletGeneratorKernelsAlloc.cc
and in CAHitNtupletGeneratorKernelsAlloc.cu
have #include "CAHitNtupletGeneratorKernelsAlloc.cc"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, updated as suggested
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34140/23367
|
please test |
@@ -42,8 +42,10 @@ | |||
|
|||
//#define VTXDEBUG | |||
|
|||
const unsigned int nTracks(const reco::Vertex &sv) { return sv.nTracks(); } | |||
const unsigned int nTracks(const reco::VertexCompositePtrCandidate &sv) { return sv.numberOfSourceCandidatePtrs(); } | |||
inline const unsigned int nTracks(const reco::Vertex &sv) { return sv.nTracks(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would embed this in place using constexpr if (std::is_same ... etc
or at least add some sort of namespace....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really a header file?
it is a producer, so it shall be final and most probably is missing just two macros...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still a function named nTracks shall not be allowed in the global namespace so inline even if it becomes a .src
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can just merge this header in to its source file ( https://github.com/cms-sw/cmssw/blob/master/RecoVertex/AdaptiveVertexFinder/plugins/VertexArbitrators.cc )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cms-sw/reconstruction-l2 , any objection on merging TemplatedVertexArbitrator.h
in to VertexArbitrators.cc
which is the only src file which includes it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's fine to merge this to RecoVertex/AdaptiveVertexFinder/plugins/VertexArbitrators.cc
IIUC, the nTracks should be moved to the anonymous namespace (the easiest; else it's is_same
with sfinae)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TemplatedVertexArbitrator.h
is now merged in to VertexArbitrators.cc
#include <TVector3.h> | ||
#include "FWCore/MessageLogger/interface/MessageLogger.h" | ||
|
||
class Conv4HitsReco { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not quite an appropriate change. This class is not a plugin. The declaration should be available in a header file.
It is kind of dead code though and is perhaps OK to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes looks like dead code, nothing is using it. +1 to drop it.
@@ -126,13 +126,13 @@ namespace { | |||
#define PRINT LogTrace("") | |||
#endif | |||
|
|||
Trajectory KFFittingSmoother::fitOne(const Trajectory& t, fitType type) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this file was not updated. From the way it is written, we either need a single include protection with e.g. a #define KFFittingSmootherESProducer_cc
in KFFittingSmootherESProducer.cc and then an ifdef in this file; or move the contents to KFFittingSmootherESProducer.cc
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34140/23465
|
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c54a17/16181/summary.html CMS StaticAnalyzer warnings: There are 7 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c54a17/16181/llvm-analysis/esrget-sa.txt for details. Comparison SummarySummary:
|
+reconstruction for #34140 3eb8cb5
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
.h
and.cc
files of pluginsRecoTracker/ConversionSeedGenerators/plugins/Conv4HitsReco.cc