Skip to content

xAODTracking,xAODCaloEvent : Clang-tidy

Same as : !35828 (closed)

but without having: https://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-control-flow.html which seems to caused more noise than being of help in the previous one....

To give a bit more context this is more or less running https://gitlab.cern.ch/ATLAS-EGamma/Software/Reconstruction/clang-tidy-checks/-/blob/977ea394888d0e1d093d12b7170c31248d99e7e9/.clang-tidy with -fix (so in some sense is also gaining some more experience/feedback on what clang-tidy does / which checks make more sense for us).

Changes should relate now to the following mainly

/scratch/anastopoulos/Athena-git/athena/Event/xAOD/xAODBase/Root/IParticleHelpers.cxx:102:13: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
      if( ! copy.size() ) {
          ~~^~~~~~~~~~~
          copy.empty()
/cvmfs/atlas-nightlies.cern.ch/repo/sw/master_Athena_x86_64-centos7-clang9-opt/2020-08-21T2130/Athena/22.0.18/InstallArea/x86_64-centos7-clang9-opt/src/Control/AthContainers/AthContainers/DataVector.h:2271:22: note: method 'DataVector'::empty() defined here
  [[nodiscard]] bool empty() const noexcept;
                     ^
/scratch/anastopoulos/Athena-git/athena/Event/xAOD/xAODBase/Root/IParticleHelpers.cxx:154:17: warning: use nullptr [modernize-use-nullptr]
         return 0;
                ^
                nullptr

/scratch/anastopoulos/Athena-git/athena/Event/xAOD/xAODCaloEvent/Root/CaloCluster_v1.cxx:34:19: warning: calling a base constructor other than the copy constructor [bugprone-copy-constructor-init]
  CaloCluster_v1::CaloCluster_v1(const CaloCluster_v1& other)

/scratch/anastopoulos/Athena-git/athena/Event/xAOD/xAODTracking/Root/TrackSummaryAccessors_v1.cxx:9:13: warning: inclusion of deprecated C++ header 'stdint.h'; consider using 'cstdint' instead [modernize-deprecated-headers]
#   include <stdint.h>
            ^~~~~~~~~~
            <cstdint>

e.g

  • use empty() rather that !size() for "empty" checks.
  • nullptr rather than NULL /0 (stricly speaking NULL is more of an issue)
  • "modern" headers ( rather than <foo.h>)

The bugrpone-copy-constuctor-init leads to the only "non 100% trivial" change. Mainly due to making clear why the suggested change is applicable.

Added

     virtual ~IParticle() = default;
     // since we have a dtor default or delete
     // the others
     // AuxElement has copy ctor and
     // assignment but not move
     IParticle() = default;
     IParticle(const IParticle&) = default;
     IParticle& operator=(const IParticle&) = default;
     IParticle(IParticle&&) = delete;
     IParticle& operator=(IParticle&&) = delete;

instead of

virtual ~IParticle() {};

Actually , since the base does not have move and we had a dtor, the moves were already "suppressed" The other methods were actually there (defaulted). So this makes things more explicit.

Since copy ctors of derived classes were using the base defaut ctor (assuming there was not default copy ctor for the base...?) there were warnings since for clang-tidy this falls under "bugprone" (using the "wrong" base ctor ..)

In short I followed this https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-five and prb is few more lines but bit more clear what is "there".

Edited by Christos Anastopoulos

Merge request reports