[cfe-dev] -fdiagnostics-format= design review : added enum to patch

[Douglas Gregor]

On May 18, 2011, at 2:14 PM, Andrew Fish wrote:

> Update patch to use an enum, per Doug's suggestion.
> 
> <fdiagnostics-formant.patch>
> 
> Andrew Fish


@@ -86,6 +90,7 @@
     ShowNames = 0;
     ShowOptionNames = 0;
     ShowCategories = 0;
+    Format = 0;
     ShowSourceRanges = 0;
     ShowParseableFixits = 0;
     VerifyDiagnostics = 0;

Please use:

  Format = Clang;

Index: lib/Frontend/CompilerInvocation.cpp
===================================================================
--- lib/Frontend/CompilerInvocation.cpp	(revision 131538)
+++ lib/Frontend/CompilerInvocation.cpp	(working copy)
@@ -273,6 +273,12 @@
     Res.push_back("-fdiagnostics-show-name");
   if (Opts.ShowOptionNames)
     Res.push_back("-fdiagnostics-show-option");
+  if (Opts.Format == 0)
+    Res.push_back("-fdiagnostics-format=clang");
+  else if (Opts.Format == 1)
+    Res.push_back("-fdiagnostics-format=msvc");
+  else if (Opts.Format == 2)
+    Res.push_back("-fdiagnostics-format=vi");

Please use the enumeration constants here.

@@ -1048,6 +1054,22 @@
       << Args.getLastArg(OPT_fdiagnostics_show_category)->getAsString(Args)
       << ShowCategory;
 
+  llvm::StringRef Format =
+    Args.getLastArgValue(OPT_fdiagnostics_format, "none");
+  if (Format == "clang" || (Format == "none" && !Args.hasArg(OPT_fms_extensions)))
+    // generic default is clang
+    Opts.Format = DiagnosticOptions::Clang;
+  else if (Format == "msvc" || (Format == "none" && Args.hasArg(OPT_fms_extensions))) {
+    // defatult to msvc form if ms_extensions are enabled
+    Opts.Format = 1;
+    Opts.ShowColumn = DiagnosticOptions::Msvc;

I think MSVC-style diagnostics should still respect -fshow-column (even if the default may be different).

+  } else if (Format == "vi") 
+    Opts.Format = DiagnosticOptions::Vi;
+  else 
+    Diags.Report(diag::err_drv_invalid_value)
+      << Args.getLastArg(OPT_fdiagnostics_format)->getAsString(Args)
+      << Format;
+  
   Opts.ShowSourceRanges = Args.hasArg(OPT_fdiagnostics_print_source_range_info);
   Opts.ShowParseableFixits = Args.hasArg(OPT_fdiagnostics_parseable_fixits);
   Opts.VerifyDiagnostics = Args.hasArg(OPT_verify);

This could be a bit clearer if written as:

  if (Format == "none") {
    // Figure out the default
  } else if (Format == "msvc") {
   // MSVC defaults
  }

etc.

Oh, and I much prefer "default" to "none".

Erik is right that this should have a test to check the resulting format.

Thanks for working on this!

	- Doug