-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45303/40710
|
A new Pull Request was created by @dan131riley for master. It involves the following packages:
@mdhildreth, @cmsbuild, @civanch can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
type ecal |
Hi @grasph can you please take a look at this PR and check that this change does not have unexpected consequences? |
Hi @thomreis, the change is safe and should not have bad consequences. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-382e2b/40075/summary.html Comparison SummarySummary:
|
+1 |
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) |
+1 |
PR description:
We are getting compilation warnings from nvcc on aarch64:
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 typeint8_t
. I'd consider this a real bug, as making assumptions about whetherchar
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 barechar
, so the ROOT type has exactly the same bug of assuming thatchar
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.