[ntuple] improve type name renormalization

[jblomer]

Add different code path for renormalizing a demangled typeid name. Such type names come from RField<T>::TypeName(). In this case, the compiler has done already most of the normalization work and we don't need to go through ROOT meta. More importantly, the result does not depend on the state of ROOT Meta, e.g. the set of loaded libraries.

There are some parts ROOT meta does that we need to repeat for this case, e.g. dropping optional template arguments of stdlib containers.

Secondly, this PR clarifies and fixes normalization of C style fixed-size arrays.

Together, this makes the new patch version 1.0.0.2 of the spec and the anchor.

This PR fixes #19116

[github-actions]

Test Results

    21 files      21 suites   3d 10h 5m 26s ⏱️
 3 211 tests  3 211 ✅ 0 💤 0 ❌
65 708 runs  65 708 ✅ 0 💤 0 ❌

Results for commit 17d162f.

♻️ This comment has been updated with latest results.

[silverweed]

Small comment + a warning from the compiler

[hahnjo]

The approach looks good to me. We need to add handling of the __1 inline namespace of libc++, and find a solution for the ATLAS DataVector with suppressed template arguments.

[ferdymercury]

Maybe related: https://its.cern.ch/jira/browse/ROOT-5862

[pcanal]

Isn't that removing the test of the actual ATLAS issue?

[pcanal]

I.e. don't we need both the original testing and a new test?

[jblomer]

The problem observed by ATLAS is tested above. This particular part tests the TypeName() function only, which in the DataVector case is different from RField<T>::GetTypeName(). What we test now (after the patch) is that TypeName() always gives the same result independent of the currently loaded libraries.

It's arguably weak for that purpose and we may drop the TypeName() part here altogether.

[pcanal]

I am missing something as I dont see anymore the test for the short name being there ... I am especially thrown off by the comment that is being removed:

Make sure autoloading happened and the rule to suppress the second template argument kicked in

which seems explicitly say that the above call and follow test are exactly for the ATLAS scenario ... and/or where should that comment move to?

[jblomer]

Line 21 is the actual test:

ASSERT_NO_THROW(entry.GetPtr<AtlasLikeDataVector<CustomStruct>>("my_field"));

We ask for a shared pointer without the extra template argument, and it works even tough on disk the extra argument is present.

[pcanal]

I am quite confused. If/since new line 24 set the expectation to recording with the long name, then line 21 will work with or without the dictionary (demangled name match the long name).

So unless we also make sure that in the test the 'current' long name does not match the long name when storing the file (I can't recall if we are doing that), then we wouldn't really test the feature ('please ignore the 2nd argument') ... (that is, unless I am still missing something)

[jblomer]

Sorry, you're right! The test didn't make much sense. I added a modified version which I hope makes more sense.

[pcanal]

At which point in the new test is the autoloading of the library triggered?

[jblomer]

Actually I'm not sure. I guess by the constructor of ROOT::RField<AtlasLikeDataVector<CustomStruct>>? But does it matter?

[pcanal]

It does in the sense that the previous code/test was checking both before and after the autoloading, ensuring that the autoloading did happen. Without these tests, the test can later be unwittingly updated to no longer test the autoloading at all (for example by explicitly linking the dictionary library) and thus removing the test of 'RNTuple use of a specific time customized by the users will ensure that the user's library is auto-loaded' (and thus increasing the possibility that we unwittingly stop supporting it)