Skip to content

Conversation

jhp-lanl
Copy link
Collaborator

@jhp-lanl jhp-lanl commented Sep 12, 2025

PR Summary

This PR creates new helper functions for indexable types that are safer to use with indexable types.

The underlying issue is that the original Get() helper function would try to use a type from the IndexableTypes namespace to get a lamda value and then it would fall back on a numerical index if that type wasn't present. The issue here is that when a types-based indexer is used, it could contain other types and we don't want to get or overwrite the value at another index.

The safeGet() and safeSet() helpers will only use a numerical index if the indexer doesn't have a data member, is_type_indexable that is true. They can also detect whether or not they are passed a null pointer and appropriately do nothing.

There are also overloads for the safe functions that can avoid numerical indices altogether and also do nothing when a null pointer is passed.

PR Checklist

TODO

  • Spiner classes
  • Helmholtz
  • Stellar Collapse
  • Electrons
  • ZSplit EOS
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
  • Ensure that any when='@main' dependencies are updated to the release version in the package.py

Copy link
Collaborator

@Yurlungur Yurlungur 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! Some minor nitpicks below. I was initially a little skeptical of the return an error code pattern but I see how makes things simpler.

@jhp-lanl
Copy link
Collaborator Author

@Yurlungur let me know what you think now

@jhp-lanl
Copy link
Collaborator Author

Whoops... NOW it's ready for final review.

Copy link
Collaborator

@Yurlungur Yurlungur 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! I still have some comments, but I consider them non-blocking. Address or not as you see fit, and merge at will (assuming tests pass on re-git).

// Same as above but causes an error condition (static or dynamic) if the value
// can't be obtained
template <AllowedIndexing AI, typename T, typename Indexer_t>
PORTABLE_FORCEINLINE_FUNCTION void SafeMustSet(Indexer_t &lambda, std::size_t const idx,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the safe Must functions be unified and return a reference? It doesn't seem like there's a good reason to split them out and that would be a bit more DRY?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point. I'll do that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay after a bit of a const correctness rabbit hole, I think I have something that works. It leverages decltype(auto) to essentially do the right thing. The value category preservation of lambda through std::forward is admittedly a bit overkill here, but maybe we someday use an on-the-fly indexer for some reason and R-value references are important. True, we could cross this bridge then, but I figured I'd be as extensible as possible now since this is behind an impl namespace anyway.

Copy link
Collaborator Author

@jhp-lanl jhp-lanl Sep 17, 2025

Choose a reason for hiding this comment

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

Ha... well I thought I had something that worked. Let me fix the CI...

Copy link
Collaborator Author

@jhp-lanl jhp-lanl Sep 17, 2025

Choose a reason for hiding this comment

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

ChatGPT:

You ran into a classic “auto/decltype(auto) + if constexpr + always-throw” corner case.

Classic...

Anyway, after some more rabbit holes with if constexpr and deduced return type, I discovered dependent_false_v<T> can be used with static_assert in if constexpr expressions to essentially give a conditional compile-time error.. So no more PORTABLE_ALWAYS_THROW_OR_ABORT in favor of a nice compile-time error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! I think the solution you landed on is quite nice in the end.

@jhp-lanl
Copy link
Collaborator Author

@Yurlungur I'm going to go ahead and merge this so I can keep working on my other branch. If there are any additional issues, I'll make MRs to address them.

All tests on re-git pass in addition to those here on github

@jhp-lanl jhp-lanl merged commit d850d0c into main Sep 17, 2025
9 checks passed
@jhp-lanl jhp-lanl deleted the jhp/spiner_indexable_types branch September 17, 2025 04:37
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.

2 participants