Skip to content

Conversation

@broxigarchen
Copy link
Contributor

@broxigarchen broxigarchen commented Nov 6, 2023

  1. Explicitly state that use non-trivially-copyable type in annotated_ptr and annotated_ref is not supported and error out if annotated_ptr is constructed with non-trivially-copyable type

  2. void is allowed on annotated_ptr as an exception

  3. Use pass-by-value in annotated_ref operator= now, and updated the spec
    this fix volatile type not supported in the following code

void func(const T& a) { ... }
volatile T b;
func(b); // this will error out

@broxigarchen broxigarchen requested a review from a team as a code owner November 6, 2023 19:11
@broxigarchen broxigarchen changed the title add support for volatile type [SYCL] Add a overload of operator=for volatile type in annotated_ref class Nov 6, 2023
@broxigarchen broxigarchen changed the title [SYCL] Add a overload of operator=for volatile type in annotated_ref class [SYCL] Add a overload of operator= for volatile type in annotated_ref class Nov 6, 2023
@broxigarchen broxigarchen requested a review from a team as a code owner November 8, 2023 21:40
@gmlueck
Copy link
Contributor

gmlueck commented Nov 14, 2023

I assume we would to the same thing here [compound assignment]

Not just that. We should do this consistently for all arguments taking const T&

Yes, I agree. However, I think the only other place there is an argument of type const T& is in the compound assignment operators.

annotated_ref doesn't own the data (like e.g. span or ptr). The assignment changes the data not what we refer to. Therefore it is correct to assign to a const annotated_ref (which is allowed by it not being const)

In this case, shouldn't we add const to all these operators for consistency?

 T operator++();
 T operator++(int);
 T operator--();
 T operator--(int);

It seems inconsistent that aptr[0] += 1 works for a const aptr but aptr[0]++ does not.

@rolandschulz
Copy link
Contributor

Yes, I agree. However, I think the only other place there is an argument of type const T& is in the compound assignment operators.

You're right.

In this case, shouldn't we add const to all these operators for consistency?

Yes. That's a bug introduced when we added those.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Runtime changes LGTM. I'll let the other's approve the extensions changes.

@broxigarchen
Copy link
Contributor Author

@intel/llvm-gatekeepers Hi can you please help to merge this? Thanks!

The CI error seems not related

@aelovikov-intel
Copy link
Contributor

Failed Tests (1):
  SYCL :: Regression/vec_rel_swizzle_ops.cpp

is already under investigation as per #12029 (comment).

I think I've seen some discussion about

Timed Out Tests (1):
  SYCL :: Basic/vector/int-convert.cpp

as well.

@aelovikov-intel aelovikov-intel merged commit 0351926 into intel:sycl Nov 29, 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.

5 participants