[RFC] Test clang-tidy --checks misc-definitions-in-headers #33366
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33366/21955
|
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: CalibCalorimetry/CastorCalib @SiewYan, @andrius-k, @ggovi, @lveldere, @sbein, @ianna, @kpedro88, @Martin-Grunewald, @rekovic, @tlampen, @alberto-sanchez, @pohsun, @santocch, @cecilecaillol, @perrotta, @civanch, @yuanchao, @makortel, @ErnestaP, @ahmad3213, @cmsbuild, @agrohsje, @fwyzard, @GurpreetSinghChahal, @smuzaffar, @Dr15Jones, @cvuosalo, @mdhildreth, @jfernan2, @slava77, @jpata, @francescobrivio, @malbouis, @ssekmen, @mkirsano, @kmaeshima, @christopheralanwest, @srimanob, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
cmsbuild
added
tests-started
tests-rejected
and removed
tests-pending
tests-started
labels
|
-1 Failed Tests: Build BuildI found compilation error when building: >> Cuda Device Link tmp/slc7_amd64_gcc900/src/RecoPixelVertexing/PixelVertexFinding/plugins/RecoPixelVertexingPixelVertexFindingPlugins/RecoPixelVertexingPixelVertexFindingPlugins_cudadlink.o >> Building edm plugin tmp/slc7_amd64_gcc900/src/RecoPixelVertexing/PixelVertexFinding/plugins/RecoPixelVertexingPixelVertexFindingPlugins/libRecoPixelVertexingPixelVertexFindingPlugins.so /cvmfs/cms-ib.cern.ch/nweek-02675/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/RecoPixelVertexing/PixelVertexFinding/plugins/RecoPixelVertexingPixelVertexFindingPlugins/PixelVertexProducerCUDA.cc.o: in function `PixelVertexProducerCUDA::produceOnCPU(edm::StreamID, edm::Event&, edm::EventSetup const&) const': PixelVertexProducerCUDA.cc:(.text+0x40): undefined reference to `gpuVertexFinder::Producer::make(TrackSoAHeterogeneousT<32768> const*, float) const' collect2: error: ld returned 1 exit status gmake: *** [tmp/slc7_amd64_gcc900/src/RecoPixelVertexing/PixelVertexFinding/plugins/RecoPixelVertexingPixelVertexFindingPlugins/libRecoPixelVertexingPixelVertexFindingPlugins.so] Error 1 Leaving library rule at src/RecoPixelVertexing/PixelVertexFinding/plugins Entering library rule at RecoPixelVertexing/PixelVertexFinding >> Compiling /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_3_X_2021-04-07-1100/src/RecoPixelVertexing/PixelVertexFinding/src/DivisiveVertexFinder.cc >> Compiling /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_3_X_2021-04-07-1100/src/RecoPixelVertexing/PixelVertexFinding/src/PVClusterComparer.cc |
| @@ -102,7 +102,7 @@ namespace gpuVertexFinder { | |||
| #endif // PIXVERTEX_DEBUG_PRODUCE | |||
| ZVertexHeterogeneous vertices(cms::cuda::make_device_unique<ZVertexSoA>(stream)); | |||
| #else | |||
| ZVertexHeterogeneous Producer::make(TkSoA const* tksoa, float ptMin) const { | |||
| inline ZVertexHeterogeneous Producer::make(TkSoA const* tksoa, float ptMin) const { | |||
There was a problem hiding this comment.
This one should be without the inline because it is effectively a source file, whose compiler is dictated by the file extension by the .cc/.cu files that include this header.
| inline ZVertexHeterogeneous Producer::make(TkSoA const* tksoa, float ptMin) const { | |
| ZVertexHeterogeneous Producer::make(TkSoA const* tksoa, float ptMin) const { // NOLINT: prevent clang-tidy from adding inline |
There was a problem hiding this comment.
Maybe we should rename this and similar files from .h to something else ?
And/or, find an other way to compile it multiple times with different compiler and compiler flags ?
There was a problem hiding this comment.
Maybe we should rename this and similar files from
.hto something else ?
That would indeed be a simple way forward.
And/or, find an other way to compile it multiple times with different compiler and compiler flags ?
With a portability technology (or at least for Kokkos or Alpaka) we'd need such a solution anyway. Then the question is whether we want to make such an exercise already with CUDA or not (I don't have a clear feeling).
There was a problem hiding this comment.
With a portability technology (or at least for Kokkos or Alpaka) we'd need such a solution anyway. Then the question is whether we want to make such an exercise already with CUDA or not (I don't have a clear feeling).
I'd say "yes, why now, if we have the time to do it now"... but I don't know if we do 🤔
There was a problem hiding this comment.
do not rename file with extensions LXR or DXR do not understand, please. already .icc or similar are a problem.
There was a problem hiding this comment.
From the core meeting: for now we could rename this kind of header to foo.cc file and then in foo.cu file do #include "foo.cc".
There was a problem hiding this comment.
anything that CORE proposes is fine with me
There was a problem hiding this comment.
OK, so we do
git rm -f RecoPixelVertexing/PixelVertexFinding/plugins/gpuVertexFinder.cc
git mv RecoPixelVertexing/PixelVertexFinding/plugins/gpuVertexFinderImpl.h RecoPixelVertexing/PixelVertexFinding/plugins/gpuVertexFinder.cc
echo '#include "gpuVertexFinder.cc"' > RecoPixelVertexing/PixelVertexFinding/plugins/gpuVertexFinder.cu
git add RecoPixelVertexing/PixelVertexFinding/plugins/gpuVertexFinder.cuThere was a problem hiding this comment.
Fixed in #33428.
Thanks Andrea!
|
What extensions are being considered ? |
From the docs
Good point, thanks, I'll add those to the list and extend the exercise. |
|
@makortel |
|
We'll discuss on this clang-tidy check in the core software meeting tomorrow. I tried to include |
|
is this PR still supposed to be open? |
|
No, it can be closed at this point. |
PR description:
This PR tests the impact of
misc-definitions-in-headersclang-tidy check (that is proposed in #33365). For more information on the check see https://releases.llvm.org/11.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/misc-definitions-in-headers.html.PR validation:
None.