Page MenuHomePhabricator

[Clang] remove text extension from diag::err_drv_invalid_value_with_suggestion
ClosedPublic

Authored by nickdesaulniers on Apr 27 2021, 11:27 AM.

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Apr 27 2021, 11:27 AM
nickdesaulniers created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 11:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@rtrieu Do we have a way appending arbitrary messages to a diagnostic template?

@rtrieu Do we have a way appending arbitrary messages to a diagnostic template?

Not an arbitrary number. The diagnostic format string is indexed, so it diagnostic string needs to know ahead of time how many arguments that will be passed to it.

The format types can be nested so you can do:

"Some types: %select{|%1|%1 %2|%1 %2 %3|%1 %2 %3 and more}0"

Then you can pass {0,1,2,3} first, then follow with (0,1,2,3) strings. And passing 4 first with 3 strings gets the "and more" attached to the end. Otherwise, the string concatenation will need to be done before passing to the diagnostic.

For this case, I think to capture the variations used, we could use:

"invalid value '%1' in '%0', %select{|for %3,}2 $plural{0:value must be %5|[1,4]:valid argument for '%0' are:%select{|%5|%5 %6|%5 %6 %7|%5 %6 %7 %8}4}4"

%0 - flag name
%1 - invalid value
%2 - if true, add extra "for %3"
%3 - string for extra "for", needed but ignored when %2 is false
%4 - count for valid arguments. If 0, arbitrary string after "value must be"
%5-%8 - valid arguments

This supports up to 4 types and an arbitrary string. Unifying the flang and clang diagnostics could simplify the string a bit more.

rsmith added a subscriber: rsmith.Apr 27 2021, 11:42 PM

Note that, per https://clang.llvm.org/docs/InternalsManual.html#the-format-string,

Diagnostics should never take random English strings as arguments: you shouldn’t use “you have a problem with %0” and pass in things like “your argument” or “your return value” as arguments.

... so this diagnostic needs to be fundamentally restructured: either it should be split into multiple diagnostics for the various individual messages, or a %select should be added with the appropriate message text, or similar.

  • rework diags
nickdesaulniers retitled this revision from remove single quotes around sugguestion diagnostic to [Clang] remove text extension from diag::err_drv_invalid_value_with_suggestion.Apr 29 2021, 4:55 PM
nickdesaulniers edited the summary of this revision. (Show Details)
nickdesaulniers added reviewers: rtrieu, rsmith.
  • fix flang diag placeholders

While I appreciate the flexibility afforded by %select; I find the suggestions unreadable.

xbolva00 added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
282

invalid value '%1' in '%0', value must be '%2' or greater

?

Current wording sounds strange imho.

clang/include/clang/Basic/DiagnosticDriverKinds.td
282

I don't care what color we paint the bikeshed, but please suggest a color if you don't like the one I've chosen.

xbolva00 added inline comments.Apr 29 2021, 5:26 PM
clang/include/clang/Basic/DiagnosticDriverKinds.td
282

?

I suggested.

rsmith added inline comments.Apr 29 2021, 5:52 PM
clang/lib/Driver/ToolChains/Clang.cpp
3115

I think there should be no leading space here; there's already a space in the format string. (By default FileCheck ignores differences in amount of whitespace which I imagine is why the test is passing. I think -strict-whitespace would catch this.)

3141

Likewise here.

  • remove extra whitespace, use xbolva00's sugguestion text
nickdesaulniers marked 4 inline comments as done.Apr 29 2021, 7:28 PM
nickdesaulniers added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
282

I'm sorry; I don't know how I misread what you said.

xbolva00 added inline comments.Apr 29 2021, 7:40 PM
clang/include/clang/Basic/DiagnosticDriverKinds.td
282

No problem, thanks :)

nickdesaulniers marked an inline comment as done.Apr 30 2021, 11:39 AM

Ready for rereview.

clang/include/clang/Basic/DiagnosticDriverKinds.td
282

Does the updated patch look good?

@FarisRehman when I run llvm-lit -vv flang/test/Driver/fixed-line-length.f90 I see:

UNSUPPORTED: Flang :: Driver/fixed-line-length.f90 (1 of 1)

This is with -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt;flang" used, and a flang binary produced. Does REQUIRES: new-flang-driver imply I should be doing something else?

xbolva00 added inline comments.Apr 30 2021, 1:05 PM
clang/include/clang/Basic/DiagnosticDriverKinds.td
282

Yeah!

@FarisRehman when I run llvm-lit -vv flang/test/Driver/fixed-line-length.f90 I see:

UNSUPPORTED: Flang :: Driver/fixed-line-length.f90 (1 of 1)

This is with -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt;flang" used, and a flang binary produced. Does REQUIRES: new-flang-driver imply I should be doing something else?

Currently flang is just a bash script that's a wrapper for the old driver. The new driver is only enabled conditionally - you have to set FLANG_BUILD_NEW_DRIVER to build it. The binary is called flang-new (hopefully soon it will be renamed as flang).

The changes in Flang LGTM, thanks for working on this!

MaskRay added inline comments.May 1 2021, 1:57 PM
clang/include/clang/Basic/DiagnosticDriverKinds.td
220

Note %0 is duplicated in valid values to '%0' - elsewhere we use , expected ...

@FarisRehman when I run llvm-lit -vv flang/test/Driver/fixed-line-length.f90 I see:

UNSUPPORTED: Flang :: Driver/fixed-line-length.f90 (1 of 1)

This is with -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt;flang" used, and a flang binary produced. Does REQUIRES: new-flang-driver imply I should be doing something else?

Currently flang is just a bash script that's a wrapper for the old driver. The new driver is only enabled conditionally - you have to set FLANG_BUILD_NEW_DRIVER to build it. The binary is called flang-new (hopefully soon it will be renamed as flang).

The changes in Flang LGTM, thanks for working on this!

$ llvm-lit -vv flang/test/Driver/fixed-line-length.f90
PASS: Flang :: Driver/fixed-line-length.f90 (1 of 1)

Nice, thanks for the instructions and info!

nickdesaulniers marked an inline comment as done.May 3 2021, 11:00 AM
xbolva00 added inline comments.May 3 2021, 11:10 AM
clang/test/Driver/stack-protector-guard.c
38

Not very happy with suggestion, maybe worse than before. It sounds to me that now this suggests

"-mstack-protector-guard-reg=fs gs" which is a bad suggestion...

Better (?):
error: invalid value {{.*}} in 'mstack-protector-guard-reg=', expected 'XX or 'YY'

as both use sites suggest two values, this wording could be good enough.

clang/test/Driver/stack-protector-guard.c
38

In the follow up commit, D100919, this flag will have 3 values.

clang/test/Driver/stack-protector-guard.c
38

Perhaps:

error: invalid value {{.*}} in 'mstack-protector-guard-reg=', expected one of: fs gs

MaskRay accepted this revision.May 3 2021, 11:13 AM

One last nit. Perhaps leave it open for two days in case other reviewers have further comments.

clang/include/clang/Basic/DiagnosticDriverKinds.td
279

The two can reuse err_drv_invalid_value_with_suggestion

This revision is now accepted and ready to land.May 3 2021, 11:13 AM
xbolva00 added inline comments.May 3 2021, 11:15 AM
clang/test/Driver/stack-protector-guard.c
38

or use select,
"expected %select('XX'| 'XX or 'YY')" for generalization

38

Yeah, good idea.

xbolva00 added inline comments.May 3 2021, 11:17 AM
clang/include/clang/Basic/DiagnosticDriverKinds.td
279

We dont need to reuse everything at all cost. Clear and understandable messages are far more important.

clang/include/clang/Basic/DiagnosticDriverKinds.td
279

Can they? If they did, then flang would hard code "'none' or a positive integer" and " or greater" which I think defeats the purpose of what @rsmith asked for when referring to https://clang.llvm.org/docs/InternalsManual.html#the-format-string.

  • "expected one of: ..."

Looks better now, thanks.

nickdesaulniers marked 7 inline comments as done.May 3 2021, 11:26 AM
nickdesaulniers added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
279

I've changed err_drv_invalid_value_with_suggestion since LGTM; PTAL. My main concern is that strings like "a positive integer" should be in the .td for translation (IIUC), not hardcoded in flang/lib/Frontend/CompilerInvocation.cpp.

clang/test/Driver/stack-protector-guard.c
38

I went with "expected one of". Reasoning: I don't think %select scales as simply in comparison. PTAL.

nickdesaulniers marked 2 inline comments as done.May 3 2021, 11:27 AM
xbolva00 added inline comments.May 3 2021, 11:30 AM
clang/test/Driver/stack-protector-guard.c
38

Your suggestion sounds good to me as well.

xbolva00 accepted this revision.May 3 2021, 11:30 AM

LG in the current state.

This revision was landed with ongoing or failed builds.May 5 2021, 11:13 AM
This revision was automatically updated to reflect the committed changes.