Skip to content

Conversation

@pcanal
Copy link
Contributor

@pcanal pcanal commented Jan 13, 2026

Given that ROOT knows about the type via their std::type_info (and their C++ names) and that we can not lookup a std::type_info from an std::type_index (but the reverse is supported by the C++ standard), we need to pass around the type_info (otherwise we need to build a map of supported types).

This solves #199

@pcanal pcanal added this to the F2: Integration release milestone Jan 13, 2026
@pcanal pcanal self-assigned this Jan 13, 2026
@pcanal pcanal added the enhancement New feature or request label Jan 13, 2026
@codecov
Copy link

codecov bot commented Jan 13, 2026

Codecov Report

❌ Patch coverage is 50.98039% with 25 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
form/root_storage/root_tbranch_container.cpp 25.00% 18 Missing and 3 partials ⚠️
form/storage/storage.cpp 57.14% 2 Missing and 1 partial ⚠️
form/storage/storage_container.cpp 0.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (50.98%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project check has failed because the head coverage (75.21%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
- Coverage   76.66%   75.21%   -1.45%     
==========================================
  Files         124      124              
  Lines        2725     2905     +180     
  Branches      475      510      +35     
==========================================
+ Hits         2089     2185      +96     
- Misses        442      501      +59     
- Partials      194      219      +25     
Flag Coverage Δ
unittests 75.21% <50.98%> (-1.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
form/form/form.cpp 74.19% <100.00%> (-2.95%) ⬇️
form/form/form.hpp 100.00% <ø> (ø)
form/form_module.cpp 84.78% <100.00%> (-2.72%) ⬇️
form/persistence/ipersistence.hpp 100.00% <ø> (ø)
form/persistence/persistence.cpp 84.31% <100.00%> (ø)
form/persistence/persistence.hpp 100.00% <ø> (ø)
form/root_storage/root_tbranch_container.hpp 100.00% <ø> (ø)
form/root_storage/root_ttree_container.cpp 46.15% <ø> (ø)
form/storage/istorage.hpp 100.00% <ø> (ø)
form/storage/storage.hpp 100.00% <ø> (ø)
... and 5 more

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c80b5a...50533c2. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
knoepfel
knoepfel previously approved these changes Jan 15, 2026
Copy link
Member

@knoepfel knoepfel left a comment

Choose a reason for hiding this comment

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

Changes look good to me, @pcanal. I defer to @gemmeren and @barnaliy for their comments on the FORM changes.

@pcanal
Copy link
Contributor Author

pcanal commented Jan 21, 2026

@gemmeren @aolivier23 @barnaliy Could you review this PR ? Thanks.

@pcanal pcanal requested a review from aolivier23 January 21, 2026 20:28
@barnaliy
Copy link
Contributor

@pcanal : This is a clean improvement - eliminating the manual type map makes the API much simpler. I particularly like the use of DemangleName(). Thanks for doing all this work!

Copy link
Contributor

@gemmeren gemmeren left a comment

Choose a reason for hiding this comment

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

Thanks Philippe,
no objections, nut FORM will have to support non-ROOT backends, w/o DemangleName...

@aolivier23
Copy link
Contributor

@gemmeren @pcanal @barnaliy Boost has a name demangler that doesn't depend on ROOT. It comes for free with something else phlex is using and seems to work with ROOT. Here's some documentation I quickly found: https://www.boost.org/doc/libs/latest/libs/core/doc/html/core/demangle.html

Will take a look at the PR tomorrow if it's not already merged. But sounds like great work!

Copy link
Contributor

@aolivier23 aolivier23 left a comment

Choose a reason for hiding this comment

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

The interface update looks good.

I think this will work with an eventual RNTuple backend but makes that a little more difficult. We have been using ROOT::RFieldBase::Create() when writing in the prototype, and that doesn't work with std::type_info as of writing. I think I see how to go through two steps using TClass to get a type name string using std::type_info. Or I could fall back to TClassEdit (or Boost) like you do.

std::type_info looks like it will work fine in reading mode. RNTupleReader::GetView() already has a std::type_info interface in the ROOT 6.36 documentation.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants