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

Simplify check for usm allocated vector iterator #1536

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Apr 25, 2024

After a hint from @akukanov, it seems there is a simpler approach to the fix provided in #1527.
Instead of specifically avoiding instantiating std::vector<T> for types T which are not valid, we can merely use std::decay_t prior to our USM iterator checks.
This will prevent any ill-formed std::vector from being instantiated, and also should not damage our ability to detect any valid USM allocated std::vector::iterator.
Any std::vector::iterator type created from a decayed value type should not match the original iterator type if std::decay has an effect on the value type. If std::decay has an effect on the value type, then it is not a valid std::vector.

This should fix the problem same issue with simpler code, and not change our behavior.

(There is no need to rush this before the freeze, the result should be identical, this is just cleaner code)

@danhoeflinger danhoeflinger force-pushed the dev/dhoeflin/simplify_const_value_type_fix branch from 900dc38 to b66ea51 Compare April 29, 2024 14:55
Signed-off-by: Dan Hoeflinger <[email protected]>
Copy link
Contributor

@akukanov akukanov 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 and clearly improves the code, thanks.

@danhoeflinger danhoeflinger merged commit 2f02621 into main Apr 29, 2024
19 of 20 checks passed
@danhoeflinger danhoeflinger deleted the dev/dhoeflin/simplify_const_value_type_fix branch April 29, 2024 18:09
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