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

Databox runtime retrieval #5709

Merged

Conversation

nilsdeppe
Copy link
Member

Proposed changes

This adds a method of getting a type-erased DataBox. Some retrieval errors become runtime, but this will allow us to move more code into cpp files, which will hopefully decrease compile time.

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

/// A list of all the mutable tags that have subitems
using mutable_item_parent_tags =
tmpl::remove_if<mutable_item_creation_tags,
tmpl::not_<tmpl::bind<detail::has_subitems, tmpl::_1>>>;
Copy link
Member

Choose a reason for hiding this comment

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

[optional] remove_if<..., not_<...>> -> filter<..., ...>. Similar a few times below.

DataBox<tmpl::list<Tags...>>::reset_compute_item() {
// reference items do not need to be reset
SPECTRE_ALWAYS_INLINE bool DataBox<tmpl::list<Tags...>>::reset_compute_item() {
// reference items do not need to be reset, but it
Copy link
Member

Choose a reason for hiding this comment

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

"but it" what?

*val = 2.5;
*str = "My Sample String 2";
},
make_not_null(&db::as_access(box)));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't have as_access, if I'm understanding the comment above correctly.

constexpr bool using_db_access = std::is_same_v<std::decay_t<T>, db::Access>;
// INFO("test mutate apply");
// auto box = db::create<
// db::AddSimpleTags<
Copy link
Member

Choose a reason for hiding this comment

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

Remove.

Copy link
Member

@kidder kidder left a comment

Choose a reason for hiding this comment

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

I have no other requested changes

@nilsdeppe nilsdeppe force-pushed the databox_runtime_retrieval branch 2 times, most recently from 7d4b056 to 949c549 Compare February 8, 2024 19:01
@nilsdeppe
Copy link
Member Author

I rebased and pushed fixups. Thanks for the reviews!

Copy link
Member

@kidder kidder left a comment

Choose a reason for hiding this comment

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

you have my permission to squash (including the additional request)

// reference items do not need to be reset, but it still needs to be handled
// here. Partly just so we get an error if we are calling this function
// incorrectly and because we need to call this function during resets
// to get dependency tracking correctly. When compute tags depend on reference
Copy link
Member

Choose a reason for hiding this comment

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

...tracking correct when compute tags...

Copy link
Member Author

Choose a reason for hiding this comment

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

I squashed this change into the fixup. Waiting for @wthrowe before squashing the fixups in

Copy link
Member

@wthrowe wthrowe 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. Squash.

@nilsdeppe
Copy link
Member Author

Squashed! Thank you again for the reviews!

@kidder kidder merged commit 24c5192 into sxs-collaboration:develop Feb 9, 2024
22 checks passed
nilsvu added a commit to nilsvu/spectre that referenced this pull request Feb 13, 2024
…box_runtime_retrieval"

This reverts commit 24c5192, reversing
changes made to b4b1603.
@nilsdeppe nilsdeppe deleted the databox_runtime_retrieval branch April 11, 2024 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants