clang check Alignment

[davidlange6]

Changes in Alignment subsystem due to clang-tidy if #21478 is merged

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22259/3432

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22259/3432/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22259/3432/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

[davidlange6]

hum, those code checks seem unlikely to compile - the clang-changes must have caused problems elsewhere (that I didn't check for)

[davidlange6]

here is an example of the bug introduced

if (newBareTkGeomPtr == bareTkGeomPtr_) { return; // already booked hists, nothing changed }

end-of-line comments are not dealt with correctly.

[fwyzard]

I've tried to reproduce this wrong substitution with a minimal test case, and I haven't been able to.
What options did you use ?

[smuzaffar]

@davidlange6 and @fwyzard this was due to cmssw build rules. We run clang-tidy with -export-fixes option and then post process the generated yaml files to

• drop suggested change for files which are not part of PR change set
• remove duplicate changes for headers files

Problem is that when we write back the yaml file (using pyYAML) then it drops the extra new line e.g. if originally we have

Diagnostics:     
  - DiagnosticName:  readability-braces-around-statements
    Message:         statement should be inside braces
    FileOffset:      566
    FilePath:        /build/muz/cf/CMSSW_10_6_X_2019-04-11-2300/src/Alignment/CocoaModel/interface/MeasurementCOPS.h
    Replacements:    
      - FilePath:        /build/muz/cf/CMSSW_10_6_X_2019-04-11-2300/src/Alignment/CocoaModel/interface/MeasurementCOPS.h
        Offset:          566
        Length:          0
        ReplacementText: ' {'
      - FilePath:        /build/muz/cf/CMSSW_10_6_X_2019-04-11-2300/src/Alignment/CocoaModel/interface/MeasurementCOPS.h
        Offset:          591
        Length:          0
        ReplacementText: '
}'

then reading it and writing back generates

Diagnostics:     
  - DiagnosticName:  readability-braces-around-statements
    Message:         statement should be inside braces
    FileOffset:      566
    FilePath:        /build/muz/cf/CMSSW_10_6_X_2019-04-11-2300/src/Alignment/CocoaModel/interface/MeasurementCOPS.h
    Replacements:    
      - FilePath:        /build/muz/cf/CMSSW_10_6_X_2019-04-11-2300/src/Alignment/CocoaModel/interface/MeasurementCOPS.h
        Offset:          566
        Length:          0
        ReplacementText: ' {'
      - FilePath:        /build/muz/cf/CMSSW_10_6_X_2019-04-11-2300/src/Alignment/CocoaModel/interface/MeasurementCOPS.h
        Offset:          591
        Length:          0
        ReplacementText: ' }'

this causes the closing braces to be on the same line. This should be fixes once we have cms-sw/cmsdist#4914 merged.

[davidlange6]

sorry - I missed this mail - I effectively did scram build code-checks USER_CODE_CHECKS_FILE=checks.txt where checks.txt has ~all cc files in Alignment/* packages and I added just readability-braces-around-statements to .clang-tidy. < ,readability-uniqueptr-delete-release, < ,readability-braces-around-statements, ---

,readability-uniqueptr-delete-release

david

…

On Feb 17, 2018, at 3:12 PM, Andrea Bocci ***@***.***> wrote: @fwyzard commented on this pull request. In Alignment/OfflineValidation/plugins/TrackerOfflineValidation.cc: > @@ -417,7 +417,7 @@ TrackerOfflineValidation::checkBookHists(const edm::EventSetup& es) { es.get<TrackerDigiGeometryRecord>().get( tkGeom_ ); const TrackerGeometry *newBareTkGeomPtr = &(*tkGeom_); - if (newBareTkGeomPtr == bareTkGeomPtr_) return; // already booked hists, nothing changed + if (newBareTkGeomPtr == bareTkGeomPtr_) { return; // already booked hists, nothing changed } I've tried to reproduce this wrong substitution with a minimal test case, and I haven't been able to. What options did you use ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[fwyzard]

Mhm, strange.

I just ran

clang-tidy --fix Alignment/OfflineValidation/plugins/TrackerOfflineValidation.cc

and I got a valid file:

@@ -417,7 +424,8 @@ TrackerOfflineValidation::checkBookHists(const edm::EventSetup& es)
 {
   es.get<TrackerDigiGeometryRecord>().get( tkGeom_ );
   const TrackerGeometry *newBareTkGeomPtr = &(*tkGeom_);
-  if (newBareTkGeomPtr == bareTkGeomPtr_) return; // already booked hists, nothing changed
+  if (newBareTkGeomPtr == bareTkGeomPtr_) { return; // already booked hists, nothing changed
+}
 
   if (!bareTkGeomPtr_) { // pointer not yet set: called the first time => book hists
 

[davidlange6]

clang-tidy does behave differently when given multiple files, though I don't see a reason it would for this test (and this file, which is source file not a header file).. I'll try some other variations..

…

On Feb 20, 2018, at 6:00 PM, Andrea Bocci ***@***.***> wrote: Mhm, strange. I just ran clang-tidy --fix Alignment/OfflineValidation/plugins/TrackerOfflineValidation.cc and I got a valid file: @@ -417,7 +424,8 @@ TrackerOfflineValidation::checkBookHists(const edm::EventSetup& es) { es.get<TrackerDigiGeometryRecord>().get( tkGeom_ ); const TrackerGeometry *newBareTkGeomPtr = &(*tkGeom_); - if (newBareTkGeomPtr == bareTkGeomPtr_) return; // already booked hists, nothing changed + if (newBareTkGeomPtr == bareTkGeomPtr_) { return; // already booked hists, nothing changed +} if (!bareTkGeomPtr_) { // pointer not yet set: called the first time => book hists — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[davidlange6]

Your PR is unmergeable. Please have a look and possibly rebase it.

[arunhep]

@davidlange6 would you like to continue with these changes? Or if not, then may be you want to close it.
thanks.

[arunhep]

ping

[arunhep]

-1
to remove it from the list