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

[Nick Desaulniers via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGaefbfbcbd776: [Clang] remove text extension from 
diag::err_drv_invalid_value_with_suggestion (authored by nickdesaulniers).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/test/Driver/fixed-line-length.f90

Index: flang/test/Driver/fixed-line-length.f90
===
--- flang/test/Driver/fixed-line-length.f90
+++ flang/test/Driver/fixed-line-length.f90
@@ -37,12 +37,12 @@
 !-
 ! EXPECTED OUTPUT WITH A NEGATIVE LENGTH
 !-
-! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=','value must be 'none' or a non-negative integer'
+! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=', value must be 'none' or a positive integer
 
 !-
 ! EXPECTED OUTPUT WITH LENGTH LESS THAN 7
 !-
-! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=','value must be at least seven'
+! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=', value must be '7' or greater
 
 !---
 ! EXPECTED OUTPUT WITH UNLIMITED LENGTH
Index: flang/lib/Frontend/CompilerInvocation.cpp
===
--- flang/lib/Frontend/CompilerInvocation.cpp
+++ flang/lib/Frontend/CompilerInvocation.cpp
@@ -259,15 +259,13 @@
   columns = -1;
 }
 if (columns < 0) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be 'none' or a non-negative integer";
+  diags.Report(clang::diag::err_drv_negative_columns)
+  << arg->getOption().getName() << arg->getValue();
 } else if (columns == 0) {
   opts.fixedFormColumns_ = 100;
 } else if (columns < 7) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be at least seven";
+  diags.Report(clang::diag::err_drv_small_columns)
+  << arg->getOption().getName() << arg->getValue() << "7";
 } else {
   opts.fixedFormColumns_ = columns;
 }
Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -7,7 +7,7 @@
 
 // CHECK-TLS: "-cc1" {{.*}}"-mstack-protector-guard=tls"
 // CHECK-GLOBAL: "-cc1" {{.*}}"-mstack-protector-guard=global"
-// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:tls global'
+// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=', expected one of: tls global
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-FS %s
@@ -35,7 +35,7 @@
 
 // CHECK-FS: "-cc1" {{.*}}"-mstack-protector-guard-reg=fs"
 // 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 one of: fs gs
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-offset=30 %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OFFSET %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3112,8 +3112,7 @@
   << A->getAsString(Args) << TripleStr;
 if (Value != "tls" && Value != "global") {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "valid arguments to '-mstack-protector-guard=' are:tls global";
+  << A->getOption().getName() << Value << "tls global";
   return;
 }
 A->render(Args, CmdArgs);
@@ -3139,8 +3138,7 @@
   << A->getAsString(Args) << TripleStr;
 if (EffectiveTriple.isX86() && (Value != "fs" && Value != "gs")) {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
+  << A->getOption().getName() << Value << "fs gs";
   return;
 }
 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 accepted this revision.
xbolva00 added a comment.

LG in the current state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



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
 

nickdesaulniers wrote:
> 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.
Your suggestion sounds good to me as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Looks better now, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 342475.
nickdesaulniers added a comment.

- "expected one of: ..."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/test/Driver/fixed-line-length.f90

Index: flang/test/Driver/fixed-line-length.f90
===
--- flang/test/Driver/fixed-line-length.f90
+++ flang/test/Driver/fixed-line-length.f90
@@ -37,12 +37,12 @@
 !-
 ! EXPECTED OUTPUT WITH A NEGATIVE LENGTH
 !-
-! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=','value must be 'none' or a non-negative integer'
+! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=', value must be 'none' or a positive integer
 
 !-
 ! EXPECTED OUTPUT WITH LENGTH LESS THAN 7
 !-
-! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=','value must be at least seven'
+! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=', value must be '7' or greater
 
 !---
 ! EXPECTED OUTPUT WITH UNLIMITED LENGTH
Index: flang/lib/Frontend/CompilerInvocation.cpp
===
--- flang/lib/Frontend/CompilerInvocation.cpp
+++ flang/lib/Frontend/CompilerInvocation.cpp
@@ -259,15 +259,13 @@
   columns = -1;
 }
 if (columns < 0) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be 'none' or a non-negative integer";
+  diags.Report(clang::diag::err_drv_negative_columns)
+  << arg->getOption().getName() << arg->getValue();
 } else if (columns == 0) {
   opts.fixedFormColumns_ = 100;
 } else if (columns < 7) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be at least seven";
+  diags.Report(clang::diag::err_drv_small_columns)
+  << arg->getOption().getName() << arg->getValue() << "7";
 } else {
   opts.fixedFormColumns_ = columns;
 }
Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -7,7 +7,7 @@
 
 // CHECK-TLS: "-cc1" {{.*}}"-mstack-protector-guard=tls"
 // CHECK-GLOBAL: "-cc1" {{.*}}"-mstack-protector-guard=global"
-// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:tls global'
+// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=', expected one of: tls global
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-FS %s
@@ -35,7 +35,7 @@
 
 // CHECK-FS: "-cc1" {{.*}}"-mstack-protector-guard-reg=fs"
 // 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 one of: fs gs
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-offset=30 %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OFFSET %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3112,8 +3112,7 @@
   << A->getAsString(Args) << TripleStr;
 if (Value != "tls" && Value != "global") {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "valid arguments to '-mstack-protector-guard=' are:tls global";
+  << A->getOption().getName() << Value << "tls global";
   return;
 }
 A->render(Args, CmdArgs);
@@ -3139,8 +3138,7 @@
   << A->getAsString(Args) << TripleStr;
 if (EffectiveTriple.isX86() && (Value != "fs" && Value != "gs")) {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
+  << A->getOption().getName() << Value << "fs gs";
   return;
 }
 A->render(Args, CmdArgs);
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- 

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

[Nick Desaulniers via Phabricator via cfe-commits]

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">;

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 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">;

MaskRay wrote:
> The two can reuse `err_drv_invalid_value_with_suggestion`
We dont need to reuse everything at all cost. Clear and understandable messages 
are far more important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



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
 

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

One last nit. Perhaps leave it open for two days in case other reviewers have 
further 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">;

The two can reuse `err_drv_invalid_value_with_suggestion`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



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
 

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



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
 

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D101387#2731436 , @awarzynski 
wrote:

> In D101387#2730043 , 
> @nickdesaulniers wrote:
>
>> @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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 342470.
nickdesaulniers added a comment.

- trim diag as per @MaskRay


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/test/Driver/fixed-line-length.f90

Index: flang/test/Driver/fixed-line-length.f90
===
--- flang/test/Driver/fixed-line-length.f90
+++ flang/test/Driver/fixed-line-length.f90
@@ -37,12 +37,12 @@
 !-
 ! EXPECTED OUTPUT WITH A NEGATIVE LENGTH
 !-
-! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=','value must be 'none' or a non-negative integer'
+! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=', value must be 'none' or a positive integer
 
 !-
 ! EXPECTED OUTPUT WITH LENGTH LESS THAN 7
 !-
-! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=','value must be at least seven'
+! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=', value must be '7' or greater
 
 !---
 ! EXPECTED OUTPUT WITH UNLIMITED LENGTH
Index: flang/lib/Frontend/CompilerInvocation.cpp
===
--- flang/lib/Frontend/CompilerInvocation.cpp
+++ flang/lib/Frontend/CompilerInvocation.cpp
@@ -259,15 +259,13 @@
   columns = -1;
 }
 if (columns < 0) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be 'none' or a non-negative integer";
+  diags.Report(clang::diag::err_drv_negative_columns)
+  << arg->getOption().getName() << arg->getValue();
 } else if (columns == 0) {
   opts.fixedFormColumns_ = 100;
 } else if (columns < 7) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be at least seven";
+  diags.Report(clang::diag::err_drv_small_columns)
+  << arg->getOption().getName() << arg->getValue() << "7";
 } else {
   opts.fixedFormColumns_ = columns;
 }
Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -7,7 +7,7 @@
 
 // CHECK-TLS: "-cc1" {{.*}}"-mstack-protector-guard=tls"
 // CHECK-GLOBAL: "-cc1" {{.*}}"-mstack-protector-guard=global"
-// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:tls global'
+// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=', expected: tls global
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-FS %s
@@ -35,7 +35,7 @@
 
 // CHECK-FS: "-cc1" {{.*}}"-mstack-protector-guard-reg=fs"
 // 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
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-offset=30 %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OFFSET %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3112,8 +3112,7 @@
   << A->getAsString(Args) << TripleStr;
 if (Value != "tls" && Value != "global") {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "valid arguments to '-mstack-protector-guard=' are:tls global";
+  << A->getOption().getName() << Value << "tls global";
   return;
 }
 A->render(Args, CmdArgs);
@@ -3139,8 +3138,7 @@
   << A->getAsString(Args) << TripleStr;
 if (EffectiveTriple.isX86() && (Value != "fs" && Value != "gs")) {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
+  << A->getOption().getName() << Value << "fs gs";
   return;
 }
 A->render(Args, CmdArgs);
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- 

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:218
+def err_drv_invalid_value_with_suggestion : Error<
+"invalid value '%1' in '%0', valid values to '%0' are: %2">;
 def err_drv_invalid_remap_file : Error<

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D101387#2730043 , @nickdesaulniers 
wrote:

> @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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:280
+def err_drv_small_columns : Error<
+"invalid value '%1' in '%0', expected '%1' to be '%2' or greater">;
 

nickdesaulniers wrote:
> xbolva00 wrote:
> > nickdesaulniers wrote:
> > > xbolva00 wrote:
> > > > nickdesaulniers wrote:
> > > > > xbolva00 wrote:
> > > > > > invalid value '%1' in '%0', value must be '%2' or greater
> > > > > > 
> > > > > > ?
> > > > > > 
> > > > > > Current wording sounds strange imho.
> > > > > 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.
> > > > ?
> > > > 
> > > > I suggested.
> > > I'm sorry; I don't know how I misread what you said.
> > No problem, thanks :)
> Does the updated patch look good?
Yeah!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

@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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.

Ready for rereview.




Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:280
+def err_drv_small_columns : Error<
+"invalid value '%1' in '%0', expected '%1' to be '%2' or greater">;
 

xbolva00 wrote:
> nickdesaulniers wrote:
> > xbolva00 wrote:
> > > nickdesaulniers wrote:
> > > > xbolva00 wrote:
> > > > > invalid value '%1' in '%0', value must be '%2' or greater
> > > > > 
> > > > > ?
> > > > > 
> > > > > Current wording sounds strange imho.
> > > > 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.
> > > ?
> > > 
> > > I suggested.
> > I'm sorry; I don't know how I misread what you said.
> No problem, thanks :)
Does the updated patch look good?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:280
+def err_drv_small_columns : Error<
+"invalid value '%1' in '%0', expected '%1' to be '%2' or greater">;
 

nickdesaulniers wrote:
> xbolva00 wrote:
> > nickdesaulniers wrote:
> > > xbolva00 wrote:
> > > > invalid value '%1' in '%0', value must be '%2' or greater
> > > > 
> > > > ?
> > > > 
> > > > Current wording sounds strange imho.
> > > 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.
> > ?
> > 
> > I suggested.
> I'm sorry; I don't know how I misread what you said.
No problem, thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:280
+def err_drv_small_columns : Error<
+"invalid value '%1' in '%0', expected '%1' to be '%2' or greater">;
 

xbolva00 wrote:
> nickdesaulniers wrote:
> > xbolva00 wrote:
> > > invalid value '%1' in '%0', value must be '%2' or greater
> > > 
> > > ?
> > > 
> > > Current wording sounds strange imho.
> > 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.
> ?
> 
> I suggested.
I'm sorry; I don't know how I misread what you said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 341742.
nickdesaulniers added a comment.

- remove extra whitespace, use xbolva00's sugguestion text


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/test/Driver/fixed-line-length.f90

Index: flang/test/Driver/fixed-line-length.f90
===
--- flang/test/Driver/fixed-line-length.f90
+++ flang/test/Driver/fixed-line-length.f90
@@ -37,12 +37,12 @@
 !-
 ! EXPECTED OUTPUT WITH A NEGATIVE LENGTH
 !-
-! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=','value must be 'none' or a non-negative integer'
+! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=', value must be 'none' or a positive integer
 
 !-
 ! EXPECTED OUTPUT WITH LENGTH LESS THAN 7
 !-
-! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=','value must be at least seven'
+! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=', value must be '7' or greater
 
 !---
 ! EXPECTED OUTPUT WITH UNLIMITED LENGTH
Index: flang/lib/Frontend/CompilerInvocation.cpp
===
--- flang/lib/Frontend/CompilerInvocation.cpp
+++ flang/lib/Frontend/CompilerInvocation.cpp
@@ -259,15 +259,13 @@
   columns = -1;
 }
 if (columns < 0) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be 'none' or a non-negative integer";
+  diags.Report(clang::diag::err_drv_negative_columns)
+  << arg->getOption().getName() << arg->getValue();
 } else if (columns == 0) {
   opts.fixedFormColumns_ = 100;
 } else if (columns < 7) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be at least seven";
+  diags.Report(clang::diag::err_drv_small_columns)
+  << arg->getOption().getName() << arg->getValue() << "7";
 } else {
   opts.fixedFormColumns_ = columns;
 }
Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -7,7 +7,7 @@
 
 // CHECK-TLS: "-cc1" {{.*}}"-mstack-protector-guard=tls"
 // CHECK-GLOBAL: "-cc1" {{.*}}"-mstack-protector-guard=global"
-// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:tls global'
+// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=', valid values to 'mstack-protector-guard=' are: tls global
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-FS %s
@@ -35,7 +35,7 @@
 
 // CHECK-FS: "-cc1" {{.*}}"-mstack-protector-guard-reg=fs"
 // 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=', valid values to 'mstack-protector-guard-reg=' are: fs gs
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-offset=30 %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OFFSET %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3112,8 +3112,7 @@
   << A->getAsString(Args) << TripleStr;
 if (Value != "tls" && Value != "global") {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "valid arguments to '-mstack-protector-guard=' are:tls global";
+  << A->getOption().getName() << Value << "tls global";
   return;
 }
 A->render(Args, CmdArgs);
@@ -3139,8 +3138,7 @@
   << A->getAsString(Args) << TripleStr;
 if (EffectiveTriple.isX86() && (Value != "fs" && Value != "gs")) {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
+  << A->getOption().getName() << Value << "fs gs";
   return;
 }
 A->render(Args, CmdArgs);
Index: 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3115
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "valid arguments to '-mstack-protector-guard=' are:tls global";
+  << A->getOption().getName() << Value << " tls global";
   return;

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.)



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3141
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs 
gs";
+  << A->getOption().getName() << Value << " fs gs";
   return;

Likewise here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:280
+def err_drv_small_columns : Error<
+"invalid value '%1' in '%0', expected '%1' to be '%2' or greater">;
 

nickdesaulniers wrote:
> xbolva00 wrote:
> > invalid value '%1' in '%0', value must be '%2' or greater
> > 
> > ?
> > 
> > Current wording sounds strange imho.
> 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.
?

I suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:280
+def err_drv_small_columns : Error<
+"invalid value '%1' in '%0', expected '%1' to be '%2' or greater">;
 

xbolva00 wrote:
> invalid value '%1' in '%0', value must be '%2' or greater
> 
> ?
> 
> Current wording sounds strange imho.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:280
+def err_drv_small_columns : Error<
+"invalid value '%1' in '%0', expected '%1' to be '%2' or greater">;
 

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

?

Current wording sounds strange imho.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 341711.
nickdesaulniers added a comment.

- fix flang diag placeholders


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101387

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/stack-protector-guard.c
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/test/Driver/fixed-line-length.f90

Index: flang/test/Driver/fixed-line-length.f90
===
--- flang/test/Driver/fixed-line-length.f90
+++ flang/test/Driver/fixed-line-length.f90
@@ -37,12 +37,12 @@
 !-
 ! EXPECTED OUTPUT WITH A NEGATIVE LENGTH
 !-
-! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=','value must be 'none' or a non-negative integer'
+! NEGATIVELENGTH: invalid value '-2' in 'ffixed-line-length=', expected '-2' to be 'none' or a positive integer
 
 !-
 ! EXPECTED OUTPUT WITH LENGTH LESS THAN 7
 !-
-! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=','value must be at least seven'
+! INVALIDLENGTH: invalid value '3' in 'ffixed-line-length=', expected '3' to be '7' or greater
 
 !---
 ! EXPECTED OUTPUT WITH UNLIMITED LENGTH
Index: flang/lib/Frontend/CompilerInvocation.cpp
===
--- flang/lib/Frontend/CompilerInvocation.cpp
+++ flang/lib/Frontend/CompilerInvocation.cpp
@@ -259,15 +259,13 @@
   columns = -1;
 }
 if (columns < 0) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be 'none' or a non-negative integer";
+  diags.Report(clang::diag::err_drv_negative_columns)
+  << arg->getOption().getName() << arg->getValue();
 } else if (columns == 0) {
   opts.fixedFormColumns_ = 100;
 } else if (columns < 7) {
-  diags.Report(clang::diag::err_drv_invalid_value_with_suggestion)
-  << arg->getOption().getName() << arg->getValue()
-  << "value must be at least seven";
+  diags.Report(clang::diag::err_drv_small_columns)
+  << arg->getOption().getName() << arg->getValue() << "7";
 } else {
   opts.fixedFormColumns_ = columns;
 }
Index: clang/test/Driver/stack-protector-guard.c
===
--- clang/test/Driver/stack-protector-guard.c
+++ clang/test/Driver/stack-protector-guard.c
@@ -7,7 +7,7 @@
 
 // CHECK-TLS: "-cc1" {{.*}}"-mstack-protector-guard=tls"
 // CHECK-GLOBAL: "-cc1" {{.*}}"-mstack-protector-guard=global"
-// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=','valid arguments to '-mstack-protector-guard=' are:tls global'
+// INVALID-VALUE: error: invalid value 'local' in 'mstack-protector-guard=', valid values to 'mstack-protector-guard=' are: tls global
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-reg=fs %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-FS %s
@@ -35,7 +35,7 @@
 
 // CHECK-FS: "-cc1" {{.*}}"-mstack-protector-guard-reg=fs"
 // 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=', valid values to 'mstack-protector-guard-reg=' are: fs gs
 
 // RUN: %clang -### -target x86_64-unknown-unknown -mstack-protector-guard-offset=30 %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=CHECK-OFFSET %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3112,8 +3112,7 @@
   << A->getAsString(Args) << TripleStr;
 if (Value != "tls" && Value != "global") {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "valid arguments to '-mstack-protector-guard=' are:tls global";
+  << A->getOption().getName() << Value << " tls global";
   return;
 }
 A->render(Args, CmdArgs);
@@ -3139,8 +3138,7 @@
   << A->getAsString(Args) << TripleStr;
 if (EffectiveTriple.isX86() && (Value != "fs" && Value != "gs")) {
   D.Diag(diag::err_drv_invalid_value_with_suggestion)
-  << A->getOption().getName() << Value
-  << "for X86, valid arguments to '-mstack-protector-guard-reg=' are:fs gs";
+  << A->getOption().getName() << Value << " fs gs";
   return;
 }
 A->render(Args, CmdArgs);
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td