[diagtool] Change default tree behavior to print only flags

[JDevlieghere]

Summary

This patch changes the default behavior of diagtool tree to only
display warning flags and not the internal warnings flags. The latter is
an implementation detail of the compiler and usually not what the users
wants.

Furthermore, flags that are enabled by default are now also printed in
green. Originally, this was only the case for the diagnostic names.

[aprantl]

Can this be tested?

tools/diagtool/ListWarningFlags.cpp

36 ↗

(On Diff #113572)

range-based for / auto?

38 ↗

(On Diff #113572)

The LLVM style wants variables to begin with an uppercase letter.

[JDevlieghere]

Thanks for the review Adrian!

I somehow completely overlooked the existing --flags-only option in diagtool tree. Obviously it makes much more sense to implement printing flags here and make it the default behavior. I've also added some code to print the flags with the appropriate color, i.e. defaults are printed in green while the others remain printed in yellow as before.

I've updated the diff to reflect this and added a test case.

[arphaman]

The diagtool tests should go to test/Misc, and there's an existing diagtool tree test there that has to be updated as well.

[JDevlieghere]

Thanks Alex!

[arphaman]

LGTM.

Please precommit the auto loop modernization though. You might want to use llvm::make_range for for-ranged loops as well.

cfe/trunk/test/Misc/warning-flags-tree.c

-// RUN: diagtool tree | FileCheck -strict-whitespace %s
-// RUN: diagtool tree -Weverything | FileCheck -strict-whitespace %s
-// RUN: diagtool tree everything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal -Weverything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal everything | FileCheck -strict-whitespace %s
 //
 // These three ways of running diagtool tree are the same:
 // they produce a tree for every top-level diagnostic flag.
 // Just check a few to make sure we're actually showing more than one group.
 //
 // CHECK: -W
 // CHECK:   -Wextra
 // CHECK:     -Wmissing-field-initializers

 // CHECK-NEXT: -Wstrict-overflow=2
 // CHECK-NEXT: -Wstrict-overflow=3
 // CHECK-NEXT: -Wstrict-overflow=4
 // CHECK-NEXT: -Wstrict-overflow=5
 
 
 // RUN: not diagtool tree -Wthis-is-not-a-valid-flag
 
-
-// RUN: diagtool tree -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
+// RUN: diagtool tree --internal -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
 // CHECK-GNU: -Wgnu
 // CHECK-GNU:   -Wgnu-designator
 // CHECK-GNU:     ext_gnu_array_range
 // CHECK-GNU:     ext_gnu_missing_equal_designator
 // CHECK-GNU:     ext_gnu_old_style_field_designator
 // CHECK-GNU:   -Wvla-extension
 // CHECK-GNU:     ext_vla
 // There are more GNU extensions but we don't need to check them all.
 
-// RUN: diagtool tree --flags-only -Wgnu | FileCheck -check-prefix CHECK-FLAGS-ONLY %s
+// RUN: diagtool tree -Wgnu | FileCheck -check-prefix CHECK-FLAGS-ONLY %s
 // CHECK-FLAGS-ONLY: -Wgnu
 // CHECK-FLAGS-ONLY:   -Wgnu-designator
 // CHECK-FLAGS-ONLY-NOT:     ext_gnu_array_range
 // CHECK-FLAGS-ONLY-NOT:     ext_gnu_missing_equal_designator
 // CHECK-FLAGS-ONLY-NOT:     ext_gnu_old_style_field_designator
 // CHECK-FLAGS-ONLY:   -Wvla
 // CHECK-FLAGS-ONLY-NOT:     ext_vla
 // CHECK-FLAGS-ONLY-NOT:   ext_array_init_copy
 // CHECK-FLAGS-ONLY-NOT:   ext_empty_struct_union
 // CHECK-FLAGS-ONLY-NOT:   ext_expr_not_ice

cfe/trunk/tools/diagtool/TreeView.cpp

if (&out != &llvm::errs() && &out != &llvm::outs())
     return false;
   return llvm::errs().is_displayed() && llvm::outs().is_displayed();
 }
 
 class TreePrinter {
 public:
   llvm::raw_ostream &out;
   const bool ShowColors;
-  bool FlagsOnly;
+  bool Internal;
 
   TreePrinter(llvm::raw_ostream &out)
-      : out(out), ShowColors(hasColors(out)), FlagsOnly(false) {}
+      : out(out), ShowColors(hasColors(out)), Internal(false) {}
 
   void setColor(llvm::raw_ostream::Colors Color) {
     if (ShowColors)
       out << llvm::sys::Process::OutputColor(Color, false, false);
   }
 
   void resetColor() {
     if (ShowColors)
       out << llvm::sys::Process::ResetColor();
   }
 
   static bool isIgnored(unsigned DiagID) {
     // FIXME: This feels like a hack.
     static clang::DiagnosticsEngine Diags(new DiagnosticIDs,
                                           new DiagnosticOptions);
     return Diags.isIgnored(DiagID, SourceLocation());
   }
 
+  static bool enabledByDefault(const GroupRecord &Group) {
+    for (const DiagnosticRecord &DR : Group.diagnostics()) {
+      if (isIgnored(DR.DiagID))
+        return false;
+    }
+
+    for (const GroupRecord &GR : Group.subgroups()) {
+      if (!enabledByDefault(GR))
+        return false;
+    }
+
+    return true;
+  }
+
   void printGroup(const GroupRecord &Group, unsigned Indent = 0) {
     out.indent(Indent * 2);
 
+    if (enabledByDefault(Group))
+      setColor(llvm::raw_ostream::GREEN);
+    else
       setColor(llvm::raw_ostream::YELLOW);
+
     out << "-W" << Group.getName() << "\n";
     resetColor();
 
     ++Indent;
     for (const GroupRecord &GR : Group.subgroups()) {
       printGroup(GR, Indent);
     }
 
-    if (!FlagsOnly) {
+    if (Internal) {
       for (const DiagnosticRecord &DR : Group.diagnostics()) {
         if (ShowColors && !isIgnored(DR.DiagID))
           setColor(llvm::raw_ostream::GREEN);
         out.indent(Indent * 2);
         out << DR.getName();
         resetColor();
         out << "\n";
       }
if (ShowColors) {
       out << "GREEN";
       resetColor();
       out << " = enabled by default\n\n";
     }
   }
 };
 
 static void printUsage() {
-  llvm::errs() << "Usage: diagtool tree [--flags-only] [<diagnostic-group>]\n";
+  llvm::errs() << "Usage: diagtool tree [--internal] [<diagnostic-group>]\n";
 }
 
 int TreeView::run(unsigned int argc, char **argv, llvm::raw_ostream &out) {
   // First check our one flag (--flags-only).
-  bool FlagsOnly = false;
+  bool Internal = false;
   if (argc > 0) {
     StringRef FirstArg(*argv);
-    if (FirstArg.equals("--flags-only")) {
-      FlagsOnly = true;
+    if (FirstArg.equals("--internal")) {
+      Internal = true;
       --argc;
       ++argv;
     }
   }
 
   bool ShowAll = false;
   StringRef RootGroup;
 
case 1:
     // FIXME: Handle other special warning flags, like -pedantic.
     break;
   default:
     printUsage();
     return -1;
   }
 
   TreePrinter TP(out);
-  TP.FlagsOnly = FlagsOnly;
+  TP.Internal = Internal;
   TP.showKey();
   return ShowAll ? TP.showAll() : TP.showGroup(RootGroup);
 }