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 #202

Closed
wdconinc opened this issue Apr 14, 2023 · 4 comments
Closed

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

wdconinc opened this issue Apr 14, 2023 · 4 comments

Comments

@wdconinc
Copy link
Contributor

There is a (potentially relevant) inconsistency between CellID in DD4hep and EDM4hep.

This leads to some ergonomics issues in downstream code where care and explicit casts are needed. I understand (of course) that changing the EDM4hep side of this is hard considering the volume of stored data files. I also understand that changing DD4hep is potentially very invasive as well. So, I guess I'm just complaining ;-)

@tmadlener
Copy link
Contributor

Argh, that seems like an oversight in the original definition. Since it is used as a bitfield (in hopefully all cases), I wouldn't rule out a change of EDM4hep a priori. On the other hand since it is used as a bitfield potentially also DD4hep could profit from at least making this a fixed width integer instead of just long long int.

Let's discuss this in a bit more detail in the next Key4hep meeting. We can then also take this to a DD4hep meeting to see how this can be reconciled.

@jmcarcell
Copy link
Member

I prefer the change from long long int to uint64_t for two reasons:

  • If you only use positive CellIDs the change from long long int to uint64_t is trivial while the opposite is not true if the CellIDs are very large
  • long long int is at least 64 bits but it may be larger on other platforms. Uncommon and unlikely, but it's still true.

@wdconinc wdconinc changed the title CellID is long long int in DD4hep, but unsigned long long in EDM4hep CellID is long long int in DD4hep, but uint64_t in EDM4hep Apr 18, 2023
@MarkusFrankATcernch
Copy link

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.

@jmcarcell
Copy link
Member

Closing since the discussion moved to AIDASoft/DD4hep#1090, which has now been fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants