[SYCL] improve operator forwarding in annotated_ref

[broxigarchen]

Consider the following case

template <typename T>
class annotated_ref {
    operator T() {}
};

template <typename T>
class A {
    template <typename T2>
    A (T2& p) {}
};

annotated_ref<A> a;
auto b = A(a);  // the operator T() from annotated_ref get shadowed by A's constructor

The fix tried to resolve this problem by making operator T() invocation to be more clear

[rolandschulz]

Style question: Does someone care whether this is instead

Suggested change

	O t2 = Ref.operator T();
	return *this = t2;
	O t2 = *(Ref.m_Ptr);
	return *this = t2;

or

Suggested change

	O t2 = Ref.operator T();
	return *this = t2;
	return *this = *(Ref.m_Ptr);;

or

Suggested change

	O t2 = Ref.operator T();
	return *this = t2;
	return *this = Ref.operator T();

[broxigarchen]

O t2 = *(Ref.m_Ptr); // this is wrong, we need the conversion to have annotation
return *this = *(Ref.m_Ptr);; // same as above

return *this = Ref.operator T(); // this is ok, I have no preference

[rolandschulz]

you're right. If someone prefers we avoid the .operator syntax we could move the annotated load into a private method in change this to

Suggested change

	O t2 = Ref.operator T();
	return *this = t2;
	return *this = Ref.load();

It might even make sense to add this to the public interface (and add it to the spec). Because user code could have the same problem and appreciate not to have to use .operator in that case.

[broxigarchen]

I will keep the .operator for now

[rolandschulz]

@Pennycook @gmlueck Do you think we should add load (or different name like get, ...) so users don't have the same problem if they have a type with conflicting conversion operator? If so we can do that in a separate PR.

[Pennycook]

Honestly, I'm not even sure I understand what these few lines are supposed to do. Could you give an example of where somebody might need to use this, outside of the definition of this operator?

[rolandschulz]

void f(annotated_ptr<MyT, ....> p) {
   MyT v = *p;
}

This works for any reasonable type. The operator* returns an annotated_ref and the assignment triggers the implicit conversion which does the annotated load. The problem is if the type is badly written and has an un/under-constrained conversion constructor e.g.:

struct MyT {
    template<class T> MyT(T&&) { ...}
};

Now that gets used rather than the implicit conversion from annotated_ref which is the intention. If a user has to deal with this kind of type (or wants to program generically very defensively that any such weird type would work) they then would instead write:

void f(annotated_ptr<MyT, ....> p) {
   MyT v = (*p).load();
}

Given that this doesn't rely on implicit conversion, there is no way for the type to interfere.
Without adding that member the user would in this case need to write:

void f(annotated_ptr<MyT, ....> p) {
   MyT v = (*p).operator MyT();
}

[Pennycook]

I see. Thanks for the explanation. I agree that adding a member function would improve usability.

My preference would be to call it load(), because that's aligned with atomic_ref (which presumably has the same problem, since it also exposes an implicit conversion to T?). For symmetry and consistency we should define store(T) as well.

[maarquitos14]

LGTM.

[broxigarchen]

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