Skip to content

Conversation

BenWibking
Copy link
Collaborator

PR Summary

When Params are created or updated, it takes ownership of the object passed with std::move.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@BenWibking BenWibking changed the title Take ownership of Params [WIP] Take ownership of Params Jun 15, 2024
@BenWibking
Copy link
Collaborator Author

BenWibking commented Jun 15, 2024

@Yurlungur is there a reason for a custom Params container? Could this be replaced with a wrapper around a std::unordered_map<std::string, std::any>?

Although I see that std::any only works for copy-constructible objects...

@BenWibking BenWibking added the experiment Outcome unknown. Some testing/eval needed. label Jun 15, 2024
@Yurlungur
Copy link
Collaborator

Thanks for taking a stab at this!

@Yurlungur is there a reason for a custom Params container? Could this be replaced with a wrapper around a std::unordered_map<std::string, std::any>?

We wrote Params when Parthenon was C++14 only. So at the time it wasn't an option. In principle, I think it could be replaced with std::any modulo the issue with copy constructible types.

I am somewhat concerned also about how this will interact with I/O. In principle it should all work though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experiment Outcome unknown. Some testing/eval needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants