This is an archive of the discontinued LLVM Phabricator instance.

Correct a lot of diagnostic wordings for the driver
ClosedPublic

Authored by aaron.ballman on Aug 3 2021, 2:22 PM.

Details

Summary

Clang diagnostics should not start with a capital letter or use trailing punctuation (https://clang.llvm.org/docs/InternalsManual.html#the-format-string), but quite a few driver diagnostics were not following this advice. This corrects the grammar and punctuation to improve consistency, but does not change the circumstances under which the diagnostics are produced.

(This hopefully shouldn't require a considerable amount of review, but I wanted the CI pipeline to run over the changes to hopefully catch any tests that don't run on Windows.)

Diff Detail

Event Timeline

aaron.ballman created this revision.Aug 3 2021, 2:22 PM
aaron.ballman requested review of this revision.Aug 3 2021, 2:22 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript

Fixing the tests I couldn't test locally but were found by the CI pipeline.

Just some thoughts! See what you like/don't like here, i'm not attached to my suggestions.

clang/include/clang/Basic/DiagnosticDriverKinds.td
86
I wonder if this should be something more like "invalid argument '<caused HIP>' not allowed in 'CUDA'".

Would be more consistent with the options mixed with C/C++ mode/etc.

88
invalid target ID %0; format is processor name followed by an optional colon delimited list of features followed by enable/disable sign, .e.g. 'gfx908:sramecc+;xnack-'

?? I think we should be leaning on the example more to explain the format.

92

This is an improvement, but this one is... rough. Not sure enough of what it is saying unfortunately to suggest a better wording.

276–277
provided host compiler IR file '%0' not found; required to generate code for OpenMP target regions

??

319–320

it 'requires' perhaps?

524

consider just removing 'the' from this and the next one.

aaron.ballman marked 5 inline comments as done.

Updating based on review feedback and restarting the CI pipeline.

clang/include/clang/Basic/DiagnosticDriverKinds.td
88

I think that's an improvement; the current wording is... hard to interpret. I tweaked it slightly, WDYT?

92

Yeah, I couldn't think of a better way to phrase it, so I figured the original wording is sufficient with some minor cleanups. I'll probably leave this one alone unless someone has a great idea.

276–277

I think the original wording reads somewhat better because it is grammatically correct. I don't think we gain a lot by replacing is with a semicolon here.

319–320

Good call!

524

Good call, but I'm also going to drop architecture from it so it's just '%0' does not support...

erichkeane accepted this revision.Aug 4 2021, 7:05 AM

The changes in this patch are all fine to me! I'm a little disturbed that it only broke 1 test though...

This revision is now accepted and ready to land.Aug 4 2021, 7:05 AM

The changes in this patch are all fine to me! I'm a little disturbed that it only broke 1 test though...

Thanks for the review!

The test coverage for driver diagnostics definitely appears to be lacking. I don't intend to add the missing test coverage myself as part of this cleanup, but when we review driver changes, we should be more insistent at requiring tests to exercise the diagnostics.

More testing fixes

Quuxplusone added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
88

Erich's suggested wording lacks the colon: invalid target ID %0, not invalid target ID: %0. I agree with Erich's suggestion.
And shouldn't it be '%0', not %0, because it's quoting user input? Compare lines 96, 98, 206, etc.

erichkeane added inline comments.Aug 4 2021, 9:13 AM
clang/include/clang/Basic/DiagnosticDriverKinds.td
88

Ah, right! I missed that in review. Yes, I think the colon doesn't add anything/should be removed. I also agree about quoting it.

Updating based on review feedback.

Updating for some test cases that are still failing.