-
Notifications
You must be signed in to change notification settings - Fork 19
Create safeGet
and safeSet
for indexable types
#564
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
Conversation
There was a problem hiding this 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.
@Yurlungur let me know what you think now |
Whoops... NOW it's ready for final review. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
… make const correct
…r_indexable_types
…time error if neither indexable types or integer indexing are allowed
@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 |
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 theIndexableTypes
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()
andsafeSet()
helpers will only use a numerical index if the indexer doesn't have a data member,is_type_indexable
that istrue
. 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
make format
command after configuring withcmake
.If preparing for a new release, in addition please check the following:
when='@main'
dependencies are updated to the release version in the package.py