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

add int64_t definition for Windows #1572

Merged
merged 1 commit into from
Jul 29, 2022
Merged

add int64_t definition for Windows #1572

merged 1 commit into from
Jul 29, 2022

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Jul 29, 2022

No description provided.

@ianna
Copy link
Collaborator Author

ianna commented Jul 29, 2022

@ManasviGoyal - could you please test that it works for you. Thanks

@ManasviGoyal
Copy link
Collaborator

ManasviGoyal commented Jul 29, 2022

@ManasviGoyal - could you please test that it works for you. Thanks

Hi @ianna. I have already made a PR for this issue. Please refer - #1571

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #1572 (719bdc1) into main (78a6535) will increase coverage by 0.26%.
The diff coverage is 76.02%.

Impacted Files Coverage Δ
src/awkward/_v2/_connect/numpy.py 63.50% <ø> (ø)
src/awkward/_v2/contents/bitmaskedarray.py 66.41% <ø> (-0.76%) ⬇️
src/awkward/_v2/contents/bytemaskedarray.py 88.82% <ø> (-0.28%) ⬇️
src/awkward/_v2/contents/emptyarray.py 71.66% <ø> (ø)
src/awkward/_v2/contents/indexedarray.py 73.83% <ø> (ø)
src/awkward/_v2/contents/listarray.py 91.79% <ø> (ø)
src/awkward/_v2/contents/listoffsetarray.py 81.85% <ø> (ø)
src/awkward/_v2/contents/numpyarray.py 87.34% <ø> (ø)
src/awkward/_v2/contents/regulararray.py 85.48% <ø> (-0.21%) ⬇️
src/awkward/_v2/contents/unionarray.py 86.27% <ø> (ø)
... and 48 more

@ianna
Copy link
Collaborator Author

ianna commented Jul 29, 2022

@ManasviGoyal - could you please test that it works for you. Thanks

Hi @ianna. I have already made a PR for this issue. Please refer - #1571

Yes, I guess we got our wires crossed :-)
I think, we use stdint.h (as in this PR) everywhere else, not cstdint. Please, let me know if this PR fixes the problem for you. Thanks.

@ianna ianna requested a review from jpivarski July 29, 2022 13:15
@ManasviGoyal
Copy link
Collaborator

ManasviGoyal commented Jul 29, 2022

@ManasviGoyal - could you please test that it works for you. Thanks

Hi @ianna. I have already made a PR for this issue. Please refer - #1571

Yes, I guess we got our wires crossed :-) I think, we use stdint.h (as in this PR) everywhere else, not cstdint. Please, let me know if this PR fixes the problem for you. Thanks.

Okay sure.

Yes it works. But I don't think we need too include it in all the files. Only adding it in BuilderOptions.h works.

Also, please cherry pick the 2nd comment from my PR. I have fixed the builder names to make them uniform throughout the tests.

@ianna
Copy link
Collaborator Author

ianna commented Jul 29, 2022

@ManasviGoyal - could you please test that it works for you. Thanks

Hi @ianna. I have already made a PR for this issue. Please refer - #1571

Yes, I guess we got our wires crossed :-) I think, we use stdint.h (as in this PR) everywhere else, not cstdint. Please, let me know if this PR fixes the problem for you. Thanks.

Okay sure.

Yes it works. But I don't think we need too include it in all the files. Only adding it in BuilderOptions.h works.

all the files need it because they use int64_t and should not rely on other includes that include this one :-)

Also, please cherry pick the 2nd comment from my PR. I have fixed the builder names to make them uniform throughout the tests.

I think, this one can be a part of your documentation PR. I'm sure there will be more fixes. Thanks!

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely an improvement. Windows is demonstrating the fact that we were relying on stdint.h without asking for it (we got it implicitly through one of the other imports).

This can be (squash-and-) merged whenever you feel you're done with it!

@ianna ianna merged commit 9e17f29 into main Jul 29, 2022
@ianna ianna deleted the ianna/include-stdint branch July 29, 2022 15:05
ManasviGoyal pushed a commit that referenced this pull request Aug 6, 2022
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

Successfully merging this pull request may close these issues.

3 participants