[PATCH] D107402: Correct a lot of diagnostic wordings for the driver

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:88
+def err_drv_bad_target_id : Error<
+  "invalid target ID: %0 (a target ID is a processor name followed by an "
+  "optional list of predefined features post-fixed by a plus or minus sign "
----------------
erichkeane wrote:
>   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.
I think that's an improvement; the current wording is... hard to interpret. I tweaked it slightly, WDYT?


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:92
+def err_drv_bad_offload_arch_combo : Error<
+  "invalid offload arch combinations: %0 and %1 (for a specific processor, a "
+  "feature should either exist in all offload archs, or not exist in any "
----------------
erichkeane wrote:
> This is an improvement, but this one is... rough.  Not sure enough of what it is saying unfortunately to suggest a better wording.
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.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:276
 def err_drv_omp_host_ir_file_not_found : Error<
-  "The provided host compiler IR file '%0' is required to generate code for OpenMP target regions but cannot be found.">;
+  "provided host compiler IR file '%0' is required to generate code for OpenMP "
+  "target regions but cannot be found">;
----------------
erichkeane wrote:
>     provided host compiler IR file '%0' not found; required to generate code for OpenMP target regions
> 
> ??
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.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:319
 def warn_drv_dwarf_version_limited_by_target : Warning<
-  "debug information option '%0' is not supported. It needs DWARF-%2 but target '%1' only provides DWARF-%3.">,
+  "debug information option '%0' is not supported; it needs DWARF-%2 but "
+  "target '%1' only provides DWARF-%3">,
----------------
erichkeane wrote:
> it 'requires' perhaps?
Good call!


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:524
 def warn_drv_moutline_unsupported_opt : Warning<
-  "The '%0' architecture does not support -moutline; flag ignored">,
+  "the '%0' architecture does not support '-moutline'; flag ignored">,
   InGroup<OptionIgnored>;
----------------
erichkeane wrote:
> consider just removing 'the' from this and the next one.
Good call, but I'm also going to drop `architecture` from it so it's just `'%0' does not support...`


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

https://reviews.llvm.org/D107402