Skip to content

[SYCL] fix a behavior mismatch in ann_ptr operator #11904

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

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

broxigarchen
Copy link
Contributor

@broxigarchen broxigarchen commented Nov 16, 2023

Fixed two issues:

  1. For compound operators, there is an error in the specs. The tmp = tmp Op rhs statement is wrong since Op is defined as += and for += it will be tmp = tmp += rhs. Correct the format
  2. For compound operators and inc/dec operators, the old implementation is using the non-compound operator which sounds wrong.
    i.e. If T is a customer type with only += operator being defined, when applying += on annotated_ref<T>, the previous implementation errors out since there is no + defined in T.
  3. Added the missing const to inc/dec operators

Copy link
Contributor

@rolandschulz rolandschulz left a comment

Choose a reason for hiding this comment

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

The version in the table is still missing the const (e.g. line 606). Also we should be more explicit about how the incr/decr works (the same operator is used on a temporary).

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

RT changes LGTM.

@broxigarchen
Copy link
Contributor Author

The version in the table is still missing the const (e.g. line 606). Also we should be more explicit about how the incr/decr works (the same operator is used on a temporary).

done

@tiwaria1
Copy link
Contributor

How was this caught? Are there any tests we can add for this and other such operator usage?

@broxigarchen
Copy link
Contributor Author

How was this caught? Are there any tests we can add for this and other such operator usage?

Found this during testing. Yes I will add it to the test code

@againull againull merged commit c43a90f into intel:sycl Nov 20, 2023
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.

6 participants