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
