Skip to content

[ntuple] Check type in RValue::GetRef<T> and GetPtr<T> #19342

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Jul 11, 2025

This requires splitting RValue and RBulkValues into a separate header. After the changes, the organization is as follows:

  • RFieldBase.hxx: defines RFieldBase and forward-declares RFieldBase::RValue and RFieldBase::RBulkValues
  • RField.hxx: includes RFieldBase.hxx and defines RField<T> with all specializations; they provide RField<T>::TypeName()
  • RFieldBaseRValue.hxx: includes both RFieldBase.hxx and RField.hxx, defines RValue and RBulkValues; the typed API uses RField<T>::TypeName()

Closes #18316

@hahnjo hahnjo requested review from jblomer, silverweed and enirolf July 11, 2025 09:52
@hahnjo hahnjo self-assigned this Jul 11, 2025
Copy link

github-actions bot commented Jul 11, 2025

Test Results

    21 files      21 suites   3d 12h 14m 52s ⏱️
 3 203 tests  3 203 ✅ 0 💤 0 ❌
65 606 runs  65 606 ✅ 0 💤 0 ❌

Results for commit 6742820.

♻️ This comment has been updated with latest results.

@hahnjo hahnjo force-pushed the ntuple-RValue-type-check branch from 4834f53 to 6742820 Compare July 15, 2025 06:39
@hahnjo hahnjo marked this pull request as ready for review July 15, 2025 06:39
if constexpr (!std::is_void_v<T>) {
if (fField->GetTypeName() != ROOT::RField<T>::TypeName()) {
throw RException(
R__FAIL("type mismatch: " + fField->GetTypeName() + " vs. " + ROOT::RField<T>::TypeName()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the error message could be a bit more descriptive, maybe something like:

Suggested change
R__FAIL("type mismatch: " + fField->GetTypeName() + " vs. " + ROOT::RField<T>::TypeName()));
R__FAIL("type mismatch for field \"" + fField->GetFieldName + "\": expected " + fField->GetTypeName() + ", got " + ROOT::RField<T>::TypeName()));

Copy link
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

Nice! Since this can break user code, I think we add an entry to the release notes.

void EnsureMatchingType() const
{
if constexpr (!std::is_void_v<T>) {
if (fField->GetTypeName() != ROOT::RField<T>::TypeName()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When #19323 is merged, I think we want to use ROOT::Internal::IsMatchingFieldType. So maybe schedule the two PRs one after the other?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes right, and we probably also want to extend roottest/root/ntuple/atlas-datavector/ to check that the procedure is working. Let's pause this PR for the moment and focus on #19323 which seems close to done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this anyway covers both RValue and RBulkValues, how about calling the header RFieldBaseValue.hxx? I think it would look slightly nicer to me.

@jblomer
Copy link
Contributor

jblomer commented Jul 16, 2025

Thinking again about it: there is a possible alternative solution using a new, internal helper class, e.g. RFieldTypeName<T>. This class would only take care of the type name and would be used both by RField<T> and RValue. The drawback is that we need to duplicate the template specializations for the new helper class but the advantage is that we don't change the user-visible includes.

@hahnjo
Copy link
Member Author

hahnjo commented Jul 16, 2025

Thinking again about it: there is a possible alternative solution using a new, internal helper class, e.g. RFieldTypeName<T>. This class would only take care of the type name and would be used both by RField<T> and RValue. The drawback is that we need to duplicate the template specializations for the new helper class but the advantage is that we don't change the user-visible includes.

Yes, it's a possibility. However, it probably requires duplicating (some of) RIntegralField and RIntegralTypeMap. We can give it a try and see if we like it better.

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

Successfully merging this pull request may close these issues.

[ntuple] RValue::GetRef<T> should type-check
3 participants