-
Notifications
You must be signed in to change notification settings - Fork 483
TPC: changing uint64_t to unsigned long long for compatibility with Mac #14399
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
Conversation
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
|
pinging @shahor02 to let you know about this problem. I will also make a new upload request for the default object and then all other pressure values |
|
@matthias-kleiner does not this make all these ccdb object backward incompatible? |
|
@shahor02 the problem is only for the pressure where we uploaded only one (the default) object. So I think it should be no problem. In any case, when I test it with the current (old) object, it still works. |
|
I don't really like this to be honest. Int64_t has a defined size. Long long int does not. So in principle, int64_t is better, and it must be safe since it has a defined size. |
|
@davidrohr ok, if it is not a problem then we can also leave it as is. On Mac I only got the warning, but the timestamps looked fine. @shahor02 do you have an opinion on this? |
|
@matthias-kleiner But also your generic DataPoint uses TimeStampType, does not this create problem? |
I think the problem is when using a |
I see that in the root-project/root#17216 suggests to use root native Uint64_t, as I expected. |
|
PS just saw your last message... |
|
I think MacOS implements int64_t differently. But basically, at every architecture the dictionary generation will see int64_t, i.e. 64 bit. Root just sees that one architecture uses long int and another long long int, but from the fact that both come from int64_t, they must be compatible by construction. |
|
I changed it now to |
shahor02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I would use it to avoid potential large CCDB retrofits in the future.
|
Error while checking build/O2/fullCI_slc9 for e095e78 at 2025-06-12 21:11: Full log here. |
|
fullCI seems to be broken due to the O2Phisics, merging after local test. |
When reading in pressure objects from Mac I got a warning of type
A similar issue is discussed here root-project/root#17216