Skip to content

[NFC][SYCL] Add/Remove punctuation #4830

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 2 commits into from
Oct 28, 2021
Merged

Conversation

elizabethandrews
Copy link
Contributor

Change diagnostic punctuation to be consistent with Clang
diagnostics

Signed-off-by: Elizabeth Andrews elizabeth.andrews@intel.com

Change diagnostic punctuation to be consistent with Clang
diagnostics

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
smanna12
smanna12 previously approved these changes Oct 27, 2021
Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

Thanks for the change.

@premanandrao
Copy link
Contributor

Are there no tests affected by this? That's a bit surprising.

@elizabethandrews
Copy link
Contributor Author

Are there no tests affected by this? That's a bit surprising.

The tests only check for the first half of diagnostic. So it doesn't fail

@premanandrao
Copy link
Contributor

Are there no tests affected by this? That's a bit surprising.

The tests only check for the first half of diagnostic. So it doesn't fail

Could we expand what it checks? It seems that the latter part of diagnostics can "slip by" otherwise.

@elizabethandrews
Copy link
Contributor Author

elizabethandrews commented Oct 27, 2021

Are there no tests affected by this? That's a bit surprising.

The tests only check for the first half of diagnostic. So it doesn't fail

Could we expand what it checks? It seems that the latter part of diagnostics can "slip by" otherwise.

I can do that. We do have a lot of tests written this way in Clang though - where you don't check the entire diagnostic because checking the first part is enough to ensure the diagnostic has been emitted. Do you want to start enforcing this in reviews going forward? I don't at the moment since it is existing practice, and personally don't feel it is required

@premanandrao
Copy link
Contributor

premanandrao commented Oct 27, 2021

Are there no tests affected by this? That's a bit surprising.

The tests only check for the first half of diagnostic. So it doesn't fail

Could we expand what it checks? It seems that the latter part of diagnostics can "slip by" otherwise.

I can do that. We do have a lot of tests written this way in Clang though - where you don't check the entire diagnostic because checking the first part is enough to ensure the diagnostic has been emitted. Do you want to start enforcing this in reviews going forward? I don't at the moment since it is existing practice, and personally don't feel it is required

What do you think about having at least one place where a diagnostic is checked in its entirety (with wildcards where appropriate), but elsewhere just enough to verify its emission, for convenience? @smanna12, your thoughts?

@smanna12
Copy link
Contributor

smanna12 commented Oct 27, 2021

Are there no tests affected by this? That's a bit surprising.

The tests only check for the first half of diagnostic. So it doesn't fail

Could we expand what it checks? It seems that the latter part of diagnostics can "slip by" otherwise.

I can do that. We do have a lot of tests written this way in Clang though - where you don't check the entire diagnostic because checking the first part is enough to ensure the diagnostic has been emitted. Do you want to start enforcing this in reviews going forward? I don't at the moment since it is existing practice, and personally don't feel it is required

What do you think about having at least one place where a diagnostic is checked in its entirety (with wildcards where appropriate), but elsewhere just enough to verify its emission, for convenience? @smanna12, your thoughts?

I personally feel to check the entire diagnostic. In case we update the diag, it would be easier to verify its emission with complete one for convenience .

Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
@elizabethandrews
Copy link
Contributor Author

Are there no tests affected by this? That's a bit surprising.

The tests only check for the first half of diagnostic. So it doesn't fail

Could we expand what it checks? It seems that the latter part of diagnostics can "slip by" otherwise.

I can do that. We do have a lot of tests written this way in Clang though - where you don't check the entire diagnostic because checking the first part is enough to ensure the diagnostic has been emitted. Do you want to start enforcing this in reviews going forward? I don't at the moment since it is existing practice, and personally don't feel it is required

What do you think about having at least one place where a diagnostic is checked in its entirety (with wildcards where appropriate), but elsewhere just enough to verify its emission, for convenience? @smanna12, your thoughts?

I personally feel to check the entire diagnostic. In case we update the diag, it would be easier to verify its emission with complete one for convenience .

Done. I added the full diagnostic. Thanks for the review!

Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

@bader bader requested a review from premanandrao October 27, 2021 23:58
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, LGTM

@bader bader merged commit 2713910 into intel:sycl Oct 28, 2021
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