Skip to content
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

DataFormats/EcalDigi: change triggerType_ and attenuation_dB_ to explicitly signed types #45303

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

dan131riley
Copy link

PR description:

We are getting compilation warnings from nvcc on aarch64:

  src/DataFormats/EcalDigi/interface/EcalMatacqDigi.h(279): warning #68-D: integer conversion resulted in a change of sign
       triggerType_ = -1;
                     ^
  src/DataFormats/EcalDigi/interface/EcalMatacqDigi.h(287): warning #68-D: integer conversion resulted in a change of sign
       attenuation_dB_ = -1;

These are being produced because those data members are declared as type char, which does not have a defined signedness, and nvcc on aarch64 defaults to unsigned char. This PR changes those values to an explicitly signed type int8_t. I'd consider this a real bug, as making assumptions about whether char is signed or not can lead to genuine logic errors.

This change does require incrementing the ClassVersion.

As an aside: since the other member data in this DataFormat use ROOT types, I considered using a signed char ROOT type--but they don't have one! Char_t claims to be signed, but it is a typedef for bare char, so the ROOT type has exactly the same bug of assuming that char has a well-defined signedness. Issue opened at root-project/root#15927 (which I expect will be ignored, as it will open a can of worms).

PR validation:

Compiles, trivial technical change except that it does bump the ClassVersion.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 25, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45303/40710

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DataFormats/EcalDigi (simulation)

@mdhildreth, @cmsbuild, @civanch can you please review it and eventually sign? Thanks.
@ReyerBand, @wang0jin, @argiro, @rovere, @missirol, @mmusich, @thomreis, @rchatter this is something you requested to watch as well.
@sextonkennedy, @rappoccio, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@thomreis
Copy link
Contributor

type ecal

@cmsbuild cmsbuild added the ecal label Jun 25, 2024
@thomreis
Copy link
Contributor

Hi @grasph can you please take a look at this PR and check that this change does not have unexpected consequences?

@grasph
Copy link
Contributor

grasph commented Jun 25, 2024

Hi @thomreis, the change is safe and should not have bad consequences.

@thomreis
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-382e2b/40075/summary.html
COMMIT: 1085691
CMSSW: CMSSW_14_1_X_2024-06-25-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45303/40075/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@civanch
Copy link
Contributor

civanch commented Jun 26, 2024

+1

@cmsbuild
Copy link
Contributor

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

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 817bd8f into cms-sw:master Jul 1, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants