[PATCH] D101387: [Clang] remove text extension from diag::err_drv_invalid_value_with_suggestion

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked 7 inline comments as done.
nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:279
   "-fembed-bitcode is not supported on versions of iOS prior to 6.0">;
+def err_drv_negative_columns : Error<
+    "invalid value '%1' in '%0', value must be 'none' or a positive integer">;
----------------
xbolva00 wrote:
> nickdesaulniers wrote:
> > MaskRay wrote:
> > > The two can reuse `err_drv_invalid_value_with_suggestion`
> > 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.
> We dont need to reuse everything at all cost. Clear and understandable messages are far more important.
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.


================
Comment at: clang/test/Driver/stack-protector-guard.c:38
 // CHECK-GS: "-cc1" {{.*}}"-mstack-protector-guard-reg=gs"
-// INVALID-REG: error: invalid value {{.*}} in 'mstack-protector-guard-reg=','for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs'
+// INVALID-REG: error: invalid value {{.*}} in 'mstack-protector-guard-reg=', expected: fs gs
 
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > xbolva00 wrote:
> > > > > 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.
> > > > In the follow up commit, D100919, this flag will have 3 values.
> > > Perhaps:
> > > 
> > > error: invalid value {{.*}} in 'mstack-protector-guard-reg=', expected one of: fs gs
> > or use select, 
> > "expected %select('XX'| 'XX or 'YY')" for generalization
> Yeah, good idea.
I went with "expected one of". Reasoning: I don't think %select scales as simply in comparison. PTAL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101387/new/

https://reviews.llvm.org/D101387