Skip to content
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

[ntuple] Remove GetView<void> with external address #18317

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Apr 8, 2025

This interface is potentially dangerous since the underlying field is constructed based on the on-disk type name, without being able to verify that the passed pointer is compatible.


I left GetView<void>("f") without external address because in principle we can do type checking there (#18316).

enirolf and others added 6 commits April 8, 2025 15:16
Enables passing a type name string to `GetView<void>` to signal that the
pointer has a different underlying type than the on-disk field's type.
The field used by `RNTupleView` will be constructed with this type, in
turn benefitting from RNTuples built-in type- and bounds-checking.
...which in turn calls the overload that takes a type name string.
This interface is potentially dangerous since the underlying field
is constructed based on the on-disk type name, without being able
to verify that the passed pointer is compatible.
@hahnjo hahnjo self-assigned this Apr 8, 2025
@hahnjo
Copy link
Member Author

hahnjo commented Apr 8, 2025

@amete @Nowakus @Dr15Jones @makortel while discussing #18185, we realized that the current interface of GetView<void>(std::string_view fieldName, void *rawPtr) is potentially dangerous because the underlying field is created based on the on-disk type name without a possibility to check that the passed pointer is compatible. I realize we are quite late here, but how bad would it be if we required passing the type name, effectively mandating #18185?

@Nowakus
Copy link
Contributor

Nowakus commented Apr 8, 2025 via email

Copy link

github-actions bot commented Apr 8, 2025

Test Results

    18 files      18 suites   5d 2h 28m 15s ⏱️
 2 719 tests  2 719 ✅ 0 💤 0 ❌
47 470 runs  47 470 ✅ 0 💤 0 ❌

Results for commit 565e4c1.

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.

3 participants