Skip to content

[ntuple] fix type names with [U]Long64_t template args#20478

Merged
jblomer merged 6 commits intoroot-project:masterfrom
jblomer:ntuple-fix-int-templ
Dec 1, 2025
Merged

[ntuple] fix type names with [U]Long64_t template args#20478
jblomer merged 6 commits intoroot-project:masterfrom
jblomer:ntuple-fix-int-templ

Conversation

@jblomer
Copy link
Contributor

@jblomer jblomer commented Nov 21, 2025

Custom classes with [U]Long64_t template arguments need to use their
meta-normalized name as type alias. Otherwise, during reconstruction
with the RNTuple normalized name, the streamer info for the
std::[u]int64_t argument will be requested (typically long
instead of long long).

Fixes #20282

@jblomer jblomer self-assigned this Nov 21, 2025
@jblomer jblomer marked this pull request as draft November 22, 2025 00:20
@github-actions
Copy link

github-actions bot commented Nov 22, 2025 •
edited
Loading

Test Results

    22 files      22 suites   4d 0h 57m 18s ⏱️
 3 780 tests  3 778 ✅ 0 💤 2 ❌
81 232 runs  81 230 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit b140667.

♻️ This comment has been updated with latest results.

@jblomer jblomer force-pushed the ntuple-fix-int-templ branch 2 times, most recently from bd12deb to be32af3 Compare November 23, 2025 21:30
@jblomer jblomer marked this pull request as ready for review November 23, 2025 22:44
@jblomer jblomer force-pushed the ntuple-fix-int-templ branch from a676f02 to 16a90ef Compare November 23, 2025 22:45
pcanal
pcanal reviewed Nov 23, 2025
View reviewed changes
pcanal
pcanal reviewed Nov 23, 2025
View reviewed changes
pcanal
pcanal reviewed Nov 23, 2025
View reviewed changes
pcanal
pcanal approved these changes Nov 23, 2025
View reviewed changes
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

LGTM

hahnjo
hahnjo reviewed Nov 24, 2025
View reviewed changes
Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

Should this be mentioned in "Type Name Normalization" of BinaryFormatSpecification.md?

@jblomer
Copy link
Contributor Author

jblomer commented Nov 26, 2025

Should this be mentioned in "Type Name Normalization" of BinaryFormatSpecification.md?

Done in the latest commit

@jblomer jblomer force-pushed the ntuple-fix-int-templ branch from 9117197 to 188b25a Compare November 26, 2025 21:40
@jblomer jblomer requested review from hahnjo and pcanal November 26, 2025 21:43
@jblomer jblomer force-pushed the ntuple-fix-int-templ branch from 188b25a to 3c9d1b0 Compare November 26, 2025 21:44
hahnjo
hahnjo approved these changes Nov 27, 2025
View reviewed changes
Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

The changes make sense to me, some comment inline

@jblomer jblomer force-pushed the ntuple-fix-int-templ branch 3 times, most recently from eaad291 to 3facd4a Compare November 27, 2025 13:18
pcanal
pcanal approved these changes Nov 27, 2025
View reviewed changes
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

Thanks.

@jblomer jblomer force-pushed the ntuple-fix-int-templ branch from 3facd4a to 1e272ec Compare November 27, 2025 14:22
@jblomer
[ntuple] add Internal::NeedsMetaNameAsAlias()
1648058
@jblomer
[ntuple] fix type names with [U]Long64_t template args
6124192 
Custom classes with [U]Long64_t template arguments need to use their
meta-normalized name as type alias. Otherwise, during reconstruction
with the RNTuple normalized name, the streamer info for the
`std::[u]int64_t` argument will be requested (typically `long`
instead of `long long`).
@jblomer
[ntuple] fix collection proxy with [U]Long64_t template arg
571411b
@jblomer
[ntuple] strip unneeded type alias arg from streamer field constructor
9a821aa
@jblomer
[ntuple] fix streamer field with [U]Long64_t template arg
cc32bb6
@jblomer
[ntuple] bump spec to version 1.0.1.1
b140667 
Clarify treatment of ROOT fundamental typedefs, including special
treatment of `[U]Long64_t`.
@jblomer jblomer force-pushed the ntuple-fix-int-templ branch from 1e272ec to b140667 Compare November 28, 2025 12:43
@jblomer jblomer merged commit b64fa6d into root-project:master Dec 1, 2025
27 of 30 checks passed
@jblomer jblomer deleted the ntuple-fix-int-templ branch December 1, 2025 12:09
@jblomer jblomer mentioned this pull request Dec 1, 2025
Merged
@makortel
Copy link

makortel commented Dec 1, 2025

Thanks for the fix! Do I understand correctly that with this PR using the Long64_t type alias, e.g. SomeTemplate<Long64_t>, with RStreamerField would work correctly, but if one would use the long long, e.g. SomeTemplate<long long> directly, that would not work but lead to similar issue we saw earlier (#20282 / cms-sw/cmssw#49298)?

(CMS' demangling should result in Long64_t in the template arguments in any case, so I'm asking just to understand the subtle details)

@makortel
Copy link

makortel commented Dec 1, 2025

Would you be (eventually) comfortable in backporting this PR to 6.36?

@jblomer jblomer mentioned this pull request Dec 1, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hahnjo hahnjo hahnjo approved these changes

@pcanal pcanal pcanal approved these changes

@silverweed silverweed Awaiting requested review from silverweed

@enirolf enirolf Awaiting requested review from enirolf

Assignees

@jblomer jblomer

Labels

in:RNTuple

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ntuple] use meta normalized name for streamer info records

4 participants

@jblomer @makortel @hahnjo @pcanal