[clang][c++20] Fixed ambiguous-reversed-operator for PixelModuleName #45492

smuzaffar · 2024-07-18T11:51:51Z

This PR should fix the clang IBs warnings [a].
@makortel @Dr15Jones , I don't know if this is the correct way to fix these warnings ... let me know if there is correct/better way

[a] https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/el8_amd64_gcc12/CMSSW_14_1_CLANG_X_2024-07-17-1700/CondFormats/SiPixelObjects

  src/CondFormats/SiPixelObjects/src/PixelFEDLink.cc:76:16: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'PixelBarrelName' and 'PixelBarrelName') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
    76 |       if (curr == prev) {
      |           ~~~~ ^  ~~~~
src/DataFormats/TrackerCommon/interface/PixelBarrelName.h:65:8: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
   65 |   bool operator==(const PixelModuleName&) const override;
      |        ^
  src/CondFormats/SiPixelObjects/src/PixelFEDLink.cc:81:18: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'PixelBarrelName' and 'PixelBarrelName') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
    81 |       if (!(curr == prev)) {
      |             ~~~~ ^  ~~~~
src/DataFormats/TrackerCommon/interface/PixelBarrelName.h:65:8: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
   65 |   bool operator==(const PixelModuleName&) const override;
      |        ^
  src/CondFormats/SiPixelObjects/src/PixelFEDLink.cc:108:18: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'PixelEndcapName' and 'PixelEndcapName') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
   108 |       if (!(curr == prev))
      |             ~~~~ ^  ~~~~
src/DataFormats/TrackerCommon/interface/PixelEndcapName.h:67:8: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
   67 |   bool operator==(const PixelModuleName&) const override;

cmsbuild · 2024-07-18T11:52:13Z

cms-bot internal usage

cmsbuild · 2024-07-18T11:54:09Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40961

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40961/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40961/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2024-07-18T12:04:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40962

cmsbuild · 2024-07-18T12:04:57Z

A new Pull Request was created by @smuzaffar for master.

It involves the following packages:

DataFormats/TrackerCommon (reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@JanFSchulte, @VinInn, @VourMa, @gpetruc, @missirol, @mmusich, @mtosi, @rovere this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2024-07-18T12:07:00Z

please test for CMSSW_14_1_CLANG_X

makortel · 2024-07-18T14:10:00Z

Adding this overload is the right way to fix the "ambiguity", but looking at the implementation of operator==(const PixelModuleName& o) functions

cmssw/DataFormats/TrackerCommon/src/PixelBarrelName.cc

Lines 722 to 729 in 1a77414

    
           bool PixelBarrelName::operator==(const PixelModuleName& o) const { 
        
             if (o.isBarrel()) { 
        
               const PixelBarrelName* other = dynamic_cast<const PixelBarrelName*>(&o); 
        
               return (other && thePart == other->thePart && theLayer == other->theLayer && theModule == other->theModule && 
        
                       theLadder == other->theLadder); 
        
             } else 
        
               return false; 
        
           }

cmssw/DataFormats/TrackerCommon/src/PixelEndcapName.cc

Lines 356 to 363 in 1a77414

    
           bool PixelEndcapName::operator==(const PixelModuleName& o) const { 
        
             if (!o.isBarrel()) { 
        
               const PixelEndcapName* other = dynamic_cast<const PixelEndcapName*>(&o); 
        
               return (other && thePart == other->thePart && theDisk == other->theDisk && theBlade == other->theBlade && 
        
                       thePannel == other->thePannel && thePlaquette == other->thePlaquette); 
        
             } else 
        
               return false; 
        
           }

it would be better to have the overloads added in this PR to have the actual implementation of the comparison, and then have the "generic" comparison operator to call these overloads depending on PixelModuleName::isBarrel().

(I somewhat wonder though if the "generic" operator is actually being used after the addition of these overloads)

cmsbuild · 2024-07-18T16:29:07Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40969

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40969/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40969/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2024-07-19T08:02:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45492/40975

cmsbuild · 2024-07-19T08:03:10Z

Pull request #45492 was updated. @civanch, @cmsbuild, @jfernan2, @mandrenguyen, @mdhildreth can you please check and sign again.

smuzaffar · 2024-07-19T08:14:04Z

please test

smuzaffar · 2024-07-19T10:23:31Z

please test for CMSSW_14_1_CLANG_X

cmsbuild · 2024-07-19T11:48:57Z

+1

Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-504be8/40491/summary.html
COMMIT: 45fe07d
CMSSW: CMSSW_14_1_X_2024-07-18-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45492/40491/install.sh to create a dev area with all the needed externals and cmssw changes.

DAS Queries: The DAS query tests failed, see the summary page for details.

Comparison Summary

Summary:

You potentially added 8 lines to the logs
Reco comparison results: 167 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3451730
DQMHistoTests: Total failures: 313
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3451397
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 206 log files, 170 edm output root files, 49 DQM output files
TriggerResults: no differences found

cmsbuild · 2024-07-19T16:34:17Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-504be8/40492/summary.html
COMMIT: 45fe07d
CMSSW: CMSSW_14_1_CLANG_X_2024-07-18-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45492/40492/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 709 lines to the logs
Reco comparison results: 37763 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3451730
DQMHistoTests: Total failures: 35567
DQMHistoTests: Total nulls: 120
DQMHistoTests: Total successes: 3416023
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.05 KiB( 48 files compared)
DQMHistoSizes: changed ( 140.043 ): -0.008 KiB JetMET/SUSYDQM
DQMHistoSizes: changed ( 140.063 ): 0.004 KiB JetMET/SUSYDQM
DQMHistoSizes: changed ( 141.042 ): 0.023 KiB JetMET/SUSYDQM
DQMHistoSizes: changed ( 141.044 ): 0.043 KiB JetMET/SUSYDQM
DQMHistoSizes: changed ( 141.046 ): -0.012 KiB JetMET/SUSYDQM
Checked 206 log files, 170 edm output root files, 49 DQM output files
TriggerResults: found differences in 7 / 47 workflows

civanch · 2024-07-19T17:22:52Z

@smuzaffar , is the warning fixed? Why we have too many difference in comparisons?

smuzaffar · 2024-07-19T17:30:13Z

@civanch , yes warnings are gone https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-504be8/40492/build-logs/.

comparison diff here #45492 (comment) are due to CLANG vs Default IB, so these are expected.
Comparison differences here #45492 (comment) are due to dd4hep workflow

civanch · 2024-07-19T19:30:20Z

+1

jfernan2 · 2024-07-22T08:19:13Z

+1

cmsbuild · 2024-07-22T08:19:35Z

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. @mandrenguyen, @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

rappoccio · 2024-07-22T18:19:06Z

+1

cmsbuild added this to the CMSSW_14_1_X milestone Jul 18, 2024

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending tracking labels Jul 18, 2024

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Jul 18, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 18, 2024

cmsbuild added tests-started and removed tests-pending labels Jul 18, 2024

cmsbuild added simulation-pending tests-pending code-checks-pending trk and removed tests-started code-checks-approved labels Jul 18, 2024

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 18, 2024

cmsbuild added the code-checks-pending label Jul 19, 2024

c++20: Fixed ambiguous-reversed-operator for PixelModuleName

45fe07d

smuzaffar force-pushed the ambiguous-reversed-operator-PixelModuleName branch from f4f9d45 to 45fe07d Compare July 19, 2024 07:57

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 19, 2024

cmsbuild added tests-started and removed tests-pending labels Jul 19, 2024

cmsbuild added tests-approved and removed tests-started labels Jul 19, 2024

cmsbuild added simulation-approved and removed simulation-pending labels Jul 19, 2024

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Jul 22, 2024

cmsbuild added orp-approved and removed orp-pending labels Jul 22, 2024

cmsbuild merged commit d6691c0 into cms-sw:master Jul 22, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][c++20] Fixed ambiguous-reversed-operator for PixelModuleName #45492

[clang][c++20] Fixed ambiguous-reversed-operator for PixelModuleName #45492

smuzaffar commented Jul 18, 2024

cmsbuild commented Jul 18, 2024 •

edited

Loading

cmsbuild commented Jul 18, 2024

cmsbuild commented Jul 18, 2024

cmsbuild commented Jul 18, 2024

smuzaffar commented Jul 18, 2024

makortel commented Jul 18, 2024

cmsbuild commented Jul 18, 2024

cmsbuild commented Jul 19, 2024

cmsbuild commented Jul 19, 2024

smuzaffar commented Jul 19, 2024

smuzaffar commented Jul 19, 2024

cmsbuild commented Jul 19, 2024

cmsbuild commented Jul 19, 2024

civanch commented Jul 19, 2024

smuzaffar commented Jul 19, 2024

civanch commented Jul 19, 2024

jfernan2 commented Jul 22, 2024

cmsbuild commented Jul 22, 2024

rappoccio commented Jul 22, 2024

[clang][c++20] Fixed ambiguous-reversed-operator for PixelModuleName #45492

[clang][c++20] Fixed ambiguous-reversed-operator for PixelModuleName #45492

Conversation

smuzaffar commented Jul 18, 2024

cmsbuild commented Jul 18, 2024 • edited Loading

cmsbuild commented Jul 18, 2024

cmsbuild commented Jul 18, 2024

cmsbuild commented Jul 18, 2024

smuzaffar commented Jul 18, 2024

makortel commented Jul 18, 2024

cmsbuild commented Jul 18, 2024

cmsbuild commented Jul 19, 2024

cmsbuild commented Jul 19, 2024

smuzaffar commented Jul 19, 2024

smuzaffar commented Jul 19, 2024

cmsbuild commented Jul 19, 2024

Comparison Summary

cmsbuild commented Jul 19, 2024

Comparison Summary

civanch commented Jul 19, 2024

smuzaffar commented Jul 19, 2024

civanch commented Jul 19, 2024

jfernan2 commented Jul 22, 2024

cmsbuild commented Jul 22, 2024

rappoccio commented Jul 22, 2024

cmsbuild commented Jul 18, 2024 •

edited

Loading