[SYCL] Move diagnostic from integration header emission

[srividya-sundaram]

This patch moves the diagnostic for the kernel name (kernel name cannot be a type in the "std" namespace) to the point in which the kernel is called, rather than during integration header emission.

[srividya-sundaram]

Question 1: The code in "sycl" branch emits the diagnostic "kernel name cannot be a type in the "std" namespace" for the following case:

typedef decltype(nullptr) nullptr_t1;
kernel_single_task<nullptr_t1>([=]() {});

Note that "nullptr_t1" is not inside a "std" namespace. The code handling the above case is here

The diagnostic message is incorrect in that the kernel name in the above example is actually not in the "std" namespace, and yet the message implies otherwise.
Instead, the message could be something along the lines of "kernel name cannot be a nullptr type". Thoughts?

Question 2: This patch has unified the include style for sycl mock header for FE tests in clang/test/CodeGenSYCL and clang/test/SemaSYCL. Is this the universal style to be followed for all FE tests?

[Fznamznon]

Question 1: The code in "sycl" branch emits the diagnostic "kernel name cannot be a type in the "std" namespace" for the following case:

typedef decltype(nullptr) nullptr_t1;
kernel_single_task<nullptr_t1>([=]() {});

Note that "nullptr_t1" is not inside a "std" namespace. The code handling the above case is here

The diagnostic message is incorrect in that the kernel name in the above example is actually not in the "std" namespace, and yet the message implies otherwise.
Instead, the message could be something along the lines of "kernel name cannot be a nullptr type". Thoughts?

Incorrectness of this diagnostic actually depends on the exact text message that this diagnostic emits. Because decltype(nullptr) is actually std::nullptr type (https://en.cppreference.com/w/cpp/language/nullptr). And it means that nullptr_t1 here is just an alias for std::nulllptr type. If the diagnostic says something like kernel name cannot be a type in the "std" namespace, note : std::nullptr type aka nullptr_t1 is used here then it should be fine, I think.

Question 2: This patch has unified the include style for sycl mock header for FE tests in clang/test/CodeGenSYCL and clang/test/SemaSYCL. Is this the universal style to be followed for all FE tests?

Yes, please.

[srividya-sundaram]

decltype(nullptr) is actually std::nullptr type

Thanks! I did not know this and was wondering why the diagnostic mentions "std" in this context?!

If the diagnostic says something like kernel name cannot be a type in the "std" namespace, note : std::nullptr type aka nullptr_t1 is used here then it should be fine, I think.

Yes, this would be a more helpful message. I will add a new entry for this, since kernel name cannot be a type in the "std" namespace is already taken for this case

[elizabethandrews]

decltype(nullptr) is actually std::nullptr type

Thanks! I did not know this and was wondering why the diagnostic mentions "std" in this context?!

If the diagnostic says something like kernel name cannot be a type in the "std" namespace, note : std::nullptr type aka nullptr_t1 is used here then it should be fine, I think.

Yes, this would be a more helpful message. I will add a new entry for this, since kernel name cannot be a type in the "std" namespace is already taken for this case

I don't think this is necessary actually. What is unclear right now? That nullptr_t1 is an alias for nullptr?

[srividya-sundaram]

decltype(nullptr) is actually std::nullptr type

Thanks! I did not know this and was wondering why the diagnostic mentions "std" in this context?!

If the diagnostic says something like kernel name cannot be a type in the "std" namespace, note : std::nullptr type aka nullptr_t1 is used here then it should be fine, I think.

Yes, this would be a more helpful message. I will add a new entry for this, since kernel name cannot be a type in the "std" namespace is already taken for this case

I don't think this is necessary actually. What is unclear right now? That nullptr_t1 is an alias for nullptr?

The keyword nullptr denotes the pointer literal. It is a prvalue of type std::nullptr_t.
The unclear part from the message is nullptr being an alias for std::nulllptr_t. Not nullptr_t1 being an alias for nullptr

[premanandrao]

The unclear part from the message is nullptr being an alias for std::nulllptr_t.

Not quite. nullptr is a value, its type is std::nullptr_t; similar to 0 being a value of type int. I don't believe this is unclear.

[srividya-sundaram]

The unclear part from the message is nullptr being an alias for std::nulllptr_t.

Not quite. nullptr is a value, its type is std::nullptr_t; similar to 0 being a value of type int. I don't believe this is unclear.

When user code has:
typedef decltype(nullptr) nullptr_t;
kernel_single_task<nullptr_t>(= {});

And the diagnostic emits a message: kernel name cannot be a type in the "std" namespace, is it obvious and clear from this message that nullptr, a pointer literal, is actually of type std::nullptr_t and hence we prohibit this? How does explicitly mentioning this in the message not helpful or unnecessary?

[premanandrao]

is it obvious and clear from this message that nullptr, a pointer literal, is actually of type std::nullptr_t and hence we prohibit this? How does explicitly mentioning this in the message not helpful or unnecessary?

Yes to the first part. The additional information does not add too much information to a user who used it in the construct to begin with.

[srividya-sundaram]

The additional information does not add too much information to a user who used it in the construct to begin with.

I don't get this statement? Also, I am not convinced with your rationale for why we shouldn't give users the exact information/reason for an error.
When a compiler emits an error message, it is expected to be user-friendly, regardless of how proficient the user might be in C++/SYCL. The message in context mentions only about "std" namespace and nothing about "nullptr". Unless there is some specific compiler rule that we must not emit helpful error message, I don't see what negative value adding this information brings. Just because it is obvious to some doesn't hurt us from adding helpful information in the message. Besides this is common paradigm in software development.

[elizabethandrews]

The additional information does not add too much information to a user who used it in the construct to begin with.

I don't get this statement? Also, I am not convinced with your rationale for why we shouldn't give users the exact information/reason for an error.

The compiler did give the exact reason for the error - i.e the kernel name, in this case nullptr_t1, has a type in std namespace. What was unclear to you was that nullptr (a C++ keyword) has type std::nullptr. If you would like to provide that as additional information, I don't see the harm. I personally don't think it is necessary but if you disagree, I have no problem with the additional note as well.

[srividya-sundaram]

The additional information does not add too much information to a user who used it in the construct to begin with.

I don't get this statement? Also, I am not convinced with your rationale for why we shouldn't give users the exact information/reason for an error.

The compiler did give the exact reason for the error - i.e the kernel name, in this case nullptr_t1, has a type in std namespace. What was unclear to you was that nullptr (a C++ keyword) has type std::nullptr. If you would like to provide that as additional information, I don't see the harm. I personally don't think it is necessary but if you disagree, I have no problem with the additional note as well.

To be precise, when the compiler says: kernel name cannot be a type in the "std" namespace, my immediate reaction is to look at the kernel name and look for any usage of "std" namespace in the kernel name. The fact that "nullptr" has a type "std:nullptr_t" which is in a std namespace and hence we prohibit its usage is not obvious from the original message. While this maybe just my ignorance, I still think it doesn't cause any harm to provide this little detail in the message we emit.
Anyways, the new message looks like this and is open for wordsmithing : kernel name cannot be a type in the "std" namespace, note: "nullptr",a pointer literal of type "std::nullptr_t" is used here"

[elizabethandrews]

Anyways, the new message looks like this and is open for wordsmithing : kernel name cannot be a type in the "std" namespace, note: "nullptr",a pointer literal of type "std::nullptr_t" is used here"

This is too long. I would prefer you add the additional information as a separate 'note' diagnostic. You can just say something like "nullptr is a prvalue of type std::nullptr_t" @premanandrao is that alright with you?

[premanandrao]

This is too long. I would prefer you add the additional information as a separate 'note' diagnostic. You can just say something like "nullptr is a prvalue of type std::nullptr_t" @premanandrao is that alright with you?

I agree that it is too long. I think a note would be helpful, but I still don't feel that saying 'nullptr is ... of type std::nullptr_t' is particularly helpful. If instead it pointed to where the type got aliased, that might be helpful.

For example:

using decltype(nullptr) my_nullptr;
class my_class { ... };
typedef my_nullclass my_class;
kernel_single_task<my_nullptr>([=]() {});  // oops, meant to say my_nullclass here

pointing out where my_nullptr is aliased to nullptr, may be helpful in the note.

[srividya-sundaram]

I prefer to keep this PR specific to improving the diagnostic message by adding an additional note. I understand that this is not everyone's personal choice, but I don't see any real problem in doing so. Re: @premanandrao's suggestion, I will consider that in a separate patch.

[Fznamznon]

Just a couple of nits to the tests, otherwise lgtm.

[elizabethandrews]

LGTM

[premanandrao]

LGTM