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] add support for std::optional<T> #15482

Merged
merged 4 commits into from May 14, 2024

Conversation

jblomer
Copy link
Contributor

@jblomer jblomer commented May 10, 2024

No description provided.

Copy link

github-actions bot commented May 11, 2024

Test Results

     9 files       9 suites   1d 16h 50m 24s ⏱️
 2 635 tests  2 635 ✅ 0 💤 0 ❌
22 356 runs  22 356 ✅ 0 💤 0 ❌

Results for commit 83438bc.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@enirolf enirolf left a comment

Choose a reason for hiding this comment

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

Looks good to me in principle! Just two nits and a question.

tree/ntuple/v7/inc/ROOT/RField.hxx Outdated Show resolved Hide resolved
tree/ntuple/v7/test/ntuple_types.cxx Outdated Show resolved Hide resolved
tree/ntuple/v7/test/ntuple_types.cxx Outdated Show resolved Hide resolved

/// Given a pointer to an std::optional<T> in from, extract a pointer to the value T* and a pointer
/// to the engagement boolean. Assumes that an std::optional<T> is stored as
/// `struct { T t; bool engagement; };`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be cases where this is not true? Can/should we check for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We rely on the unit tests to catch when this is not the case. I added some more testing on the object sizes and alignments.

@jblomer jblomer merged commit 5cc9862 into root-project:master May 14, 2024
12 of 14 checks passed
@jblomer jblomer deleted the ntuple-optional branch May 14, 2024 06:42
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.

None yet

2 participants