Skip to content

[cppyy] Add converters and low-level views for fixed width integers #18492

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

Merged
merged 2 commits into from
May 12, 2025

Conversation

aaronj0
Copy link
Contributor

@aaronj0 aaronj0 commented Apr 24, 2025

fixes: #17841

The support for the fixed-width types int16_t and int32_t indeed diverges from upstream cppyy and uses unsigned long which triggers the incorrect numpy view. Despite not having converters for the same, upstream relies on patches (or older behaviour) along the Cppyy::ResolveName -> TClassEdit::ResolveTypedef code path (see ResolveTypeDefImpl) that resolved fixed width integer types to their standard type counterparts (int16_t[][] to short[][] and int32_t[][] to int[][]) when creating converters. Trying to follow the same code branch in ROOT can have a large blast radius in the type system. Two non-intrusive ways to fix this issue are either aliasing to existing short and int converters, or adding the converters and low-level views for both types. This PR performs the latter

@aaronj0 aaronj0 requested review from guitargeek, dpiparo and vepadulano and removed request for dpiparo and vepadulano April 24, 2025 11:59
@aaronj0 aaronj0 self-assigned this Apr 24, 2025
@aaronj0 aaronj0 marked this pull request as draft April 24, 2025 12:13
Copy link

github-actions bot commented Apr 24, 2025

Test Results

    19 files      19 suites   4d 16h 39m 46s ⏱️
 2 740 tests  2 740 ✅ 0 💤 0 ❌
50 623 runs  50 623 ✅ 0 💤 0 ❌

Results for commit 7b1108b.

♻️ This comment has been updated with latest results.

@aaronj0 aaronj0 marked this pull request as ready for review April 28, 2025 09:12
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thank you very much for implementing better support for fixed-width integers! Looks good to me, but please also add a test 🙂

@guitargeek
Copy link
Contributor

Oh and don't forget to fix also the compiler warnings, otherwise the debian125 build that treats warnings as errors because of dev=ON will fail

@aaronj0 aaronj0 force-pushed the fix-array-types branch from 57c2724 to 0bc9279 Compare May 5, 2025 14:45
@aaronj0 aaronj0 requested a review from bellenot as a code owner May 5, 2025 14:45
@aaronj0 aaronj0 force-pushed the fix-array-types branch 2 times, most recently from d051d60 to 10be20c Compare May 8, 2025 09:04
@aaronj0
Copy link
Contributor Author

aaronj0 commented May 8, 2025

The following issue: #9809 causes failures on windows x86, where the arrays assigned on the C++ side itself are indexed/read wrongly:

root [0] int test[]={2,4,6,8,10};
root [1] test[4]
(int) 2
root [2] test[4]=20;
root [3] test[4]
(int) 20
root [4] test[0]
(int) 20
root [5] test
(int [5]) { 20, 4, 6, 8, 10 }
root [6]

Hence validating those buffers fail and the test will have to be disabled on win x86.

@aaronj0 aaronj0 force-pushed the fix-array-types branch from 10be20c to eb1d6ec Compare May 8, 2025 09:08
@aaronj0
Copy link
Contributor Author

aaronj0 commented May 8, 2025

Looks like the failures on win 64 are unrelated: roottest-root-io-stdpair

@aaronj0 aaronj0 force-pushed the fix-array-types branch from eb1d6ec to 7b1108b Compare May 8, 2025 18:20
@aaronj0 aaronj0 merged commit 84d61dd into root-project:master May 12, 2025
41 of 43 checks passed
@aaronj0 aaronj0 deleted the fix-array-types branch June 21, 2025 15:31
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.

PyROOT multidimenstional int16_t arrays stopped working in 6.34
2 participants