-
Notifications
You must be signed in to change notification settings - Fork 82
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
fix: remove Long64_t from common header-only #2084
fix: remove Long64_t from common header-only #2084
Conversation
…e-and-cppyy-shouldnt-be-in-awkwardheader-only
ca53e2a
to
dbe5dc6
Compare
…e-and-cppyy-shouldnt-be-in-awkwardheader-only
…e-and-cppyy-shouldnt-be-in-awkwardheader-only
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.
@jpivarski - I think, using a fundamental type here is ok for either ROOT users or those who don't have it installed. The Long64_t
is a type alias for a long
type (that is also an int64_t
) on Mac, but it's a long long
on Ubuntu.
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.
Looks good! I'd accept it as it is, but I made a suggestion to consider before merging it. Whether you decide to accept the suggestion or not, you can merge this PR.
If you merge it today (i.e. in the next few hours), then it will be included in the next awkward-cpp
release. Otherwise, we'll do another awkward-cpp
release later.
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.
@jpivarski - please, check. I'm done with this PR for today. Thanks!
…e-and-cppyy-shouldnt-be-in-awkwardheader-only
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.
See below. I'll try committing this change and auto-merging.
…e-and-cppyy-shouldnt-be-in-awkwardheader-only
Okay, I'm going to check it out locally and see what's happening. |
But this should do it:
I'm updating the code. |
…ze and signedness.
…e-and-cppyy-shouldnt-be-in-awkwardheader-only
This is working for me, locally: awkward/header-only/awkward/utils.h Lines 19 to 77 in 7929f0b
|
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.
@jpivarski - thanks! It looks great. I will try to run more extensive tests to check the containers of strings/chars.
yes, because in this case:
|
awkward/header-only
. #2032