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

CellID is long long int in DD4hep, but uint64_t in EDM4hep #1090

Closed
wdconinc opened this issue Apr 18, 2023 · 6 comments
Closed

CellID is long long int in DD4hep, but uint64_t in EDM4hep #1090

wdconinc opened this issue Apr 18, 2023 · 6 comments
Labels
bug Fixed Problem is solved Testing

Comments

@wdconinc
Copy link
Contributor

This issue is mainly to point out to this community a relevant issue on the EDM4hep issue tracker: key4hep/EDM4hep#202

Discussion on the EDM4hep issue: key4hep/EDM4hep#202.

In particular, I'd be interested in knowing when (if ever) the signed nature of the CellID could be important.

@wdconinc wdconinc added the bug label Apr 18, 2023
@wdconinc
Copy link
Contributor Author

To keep the discussion organized and in one place, please add comment to the EDM4hep issue, key4hep/EDM4hep#202.

@MarkusFrankATcernch
Copy link
Contributor

The signed'ness is probably not the issue, but it may be more interesting to specify the exact bitfield width already in the type rather rely on a less strict definition of "long long".
Probably more an academic issue: x86 will be around for a long time.

@MarkusFrankATcernch
Copy link
Contributor

MarkusFrankATcernch commented Apr 24, 2023

@wdconinc During last meeting it was decided to standardize on uint64_t for the definition of VolumeID and CellID.
It was unclear however, if really the only difference was the change in Primitives.h. Due to this uncertainty the actual time when to switch was left open.

@wdconinc
Copy link
Contributor Author

I expected that this would have some significant impacts throughout the code.

@MarkusFrankATcernch
Copy link
Contributor

@wdconinc Was not entirely clear. Some voices claimed that the value was only accessed as a bit-field and there the interpretation as signed or unsigned should not matter since signed long long and uint64_t on linux x86 has the same width.
Clearly, print statements (and if used, loop variables) will need to change.

MarkusFrankATcernch added a commit to MarkusFrankATcernch/DD4hep that referenced this issue Apr 25, 2023
MarkusFrankATcernch added a commit to MarkusFrankATcernch/DD4hep that referenced this issue Apr 25, 2023
MarkusFrankATcernch added a commit to MarkusFrankATcernch/DD4hep that referenced this issue Apr 25, 2023
MarkusFrankATcernch added a commit to MarkusFrankATcernch/DD4hep that referenced this issue Apr 27, 2023
@MarkusFrankATcernch MarkusFrankATcernch added Testing Fixed Problem is solved labels Jun 6, 2023
@MarkusFrankATcernch
Copy link
Contributor

In case of problems, please re-open the pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixed Problem is solved Testing
Projects
None yet
Development

No branches or pull requests

2 participants