[ntuple] automatic evolution in and out std::atomic #19937
Conversation
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
9ce4dd5 to
ac97cad
Compare
Test Results 7 files 7 suites 1d 9h 39m 9s ⏱️ Results for commit 1aec090. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
In principle, the changes look neat and small. I'm wondering about the error case: When reconciling an RAtomicField with something on-disk that is not std::atomic, we unconditionally delegate to the item field. Is that maybe confusing for the error message? (I don't have an immediate idea how to do it better)
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
ac97cad to
73ef349
Compare
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
73ef349 to
ef0feec
Compare
Allow for std::atomic<T> --> T (or compatible) automatic conversion and vice versa.
jblomer
force-pushed
the
ntuple-evolution-atomic
branch
from
ef0feec to
1aec090
Compare
| @@ -37,7 +37,7 @@ ROOT_GENERATE_DICTIONARY(RNTupleDescriptorDict ${CMAKE_CURRENT_SOURCE_DIR}/RNTup | |||
| DEPENDENCIES RIO) | |||
|
|
|||
| ROOT_ADD_GTEST(ntuple_endian ntuple_endian.cxx LIBRARIES ROOTNTuple) | |||
| ROOT_ADD_GTEST(ntuple_evolution_type ntuple_evolution_type.cxx LIBRARIES ROOTNTuple) | |||
| ROOT_ADD_GTEST(ntuple_evolution_type ntuple_evolution_type.cxx LIBRARIES ROOTNTuple atomic) | |||
There was a problem hiding this comment.
Should this use ROOT_ATOMIC_LIBS? Not sure...
There was a problem hiding this comment.
I think it shouldn't. ROOT_ATOMIC_LIBS contains atomic when it is needed even for the normally lock-free cases, AFAICS.
There was a problem hiding this comment.
That is not my reading. It seems to be literally that ROOT_ATOMIC_LIBS is the result of a test that checks whether std::atomic linking require an 'atomic' library or not.
| if (fieldDesc.GetTypeName().rfind("std::atomic<", 0) == 0) { | ||
| EnsureMatchingOnDiskField(fieldDesc, kDiffTypeName).ThrowOnError(); | ||
| } else { | ||
| fSubfields[0]->SetOnDiskId(GetOnDiskId()); |
There was a problem hiding this comment.
I am missing (not seeing) the part of the code that make sure that the non-atomic value is compatible with the in memory type. For example:
auto v10 = reader->GetView<CustomAtomicNotLockFree>("atomicInt");
in the new test should still throw.
Allow for std::atomic --> T (or compatible) automatic conversion and vice versa.