Accept "-Weverything" in pragma clang diagnostic ...

[Sunil_Srivastava]

Summary

Special handling for “-Weverything” in ‘pragma clang diagnostic' handling.
There is no formal diagnostic group named “everything”, so this code is needed.

[rsmith]

lib/Basic/Diagnostic.cpp

251–257

If you want to handle this at the DiagnosticsEngine level, please do so consistently: teach getDiagnosticsInGroup about this special case too, and remove the now-redundant code in clang::ProcessWarningOptions.

This is not currently setting the EnableAllWarnings flag correctly on the DiagnosticsEngine.

test/Frontend/Peverything.cpp

1 ↗

(On Diff #41448)

This test belongs in test/Preprocessor/pragma_diagnostic.c.

2 ↗

(On Diff #41448)

Please also test that #pragma clang diagnostic push/pop work for this flag.

[Sunil_Srivastava]

Please see the comment about getAllDiagnostics

lib/Basic/Diagnostic.cpp

251–257

Hi Richard, There is a problem in teaching getDiagnosticsInGroup this special case.

getDiagnosticsInGroup can get the list from getAllDiagnostics, but that list will contain disgnostics that can not be downgraded (or those for which isBuiltinWarningOrExtension() is false).

Back in setSeverityForGroup, it is safe to call setSeverityForAll, because it checks isBuiltinWarningOrExtension before calling setSeverity, but the loop in setSeverityForGroup itself does not have this check. So a simplistic getAllDiagnostics for "everything" leads to an assertion failure "Cannot map errors into warnings!" in setSeverity.

In fact, ProcessWarningOptions has the same issue because it also calls setSeverityForGroup.

So the options are:

1. add isBuiltinWarning... test in the loop in setSeverityForGroup, similar to setSeverityForAll
2. have some variant of getAllDiagnostics that returns a trimmed down list.

Please advise.

The other point about EnableAllWarnings: I agree.

test/Frontend/Peverything.cpp

1 ↗

(On Diff #41448)

OK. And I will add push pop tests as well

[Sunil_Srivastava]

Changed the test, but the compiler code is still same, pending reply from Richard Smith

[Sunil_Srivastava]

Richard, Your comment and my concern about the getDiagnosticsInGroup is still visible in the greyed out area.

Given that do you still want to modify getDiagnosticsInGroup ?

I have removed the separate test and added the new tests to existing files, as you suggested.

[rsmith]

There seem to be two principled approaches here:

1. DiagnosticsEngine has a Group named "everything" (all of its interface supports that group, and from the point of view of someone using DiagnosticsEngine it behaves like any other group), or
2. There is no such group, and everyone calling into DiagnosticsEngine treats it as a special case if they want to allow it.

This patch is more ad-hoc: some of the DiagnosticsEngine interface would allow "everything", but other parts would reject it, and we would still have a special case for "everything" in clang::ProcessWarningOptions. That results in a confusing interface contract for DiagnosticsEngine.

Can you move the special case code out of DiagnosticsEngine and into the pragma handler for now?

[Sunil_Srivastava]

Hi Richard,

Can you move the special case code out of DiagnosticsEngine and into the pragma handler for now?

Yes. This is that approach.

[rsmith]

LGTM

Please also add a test that a -Weverything on the command line can be overridden by a #pragma clang diagnostic ignored "-Weverything", even for a diagnostic with no warning flag. (The command line flag sets the EnableAllWarnings flag on the DiagnosticsEngine, and we should make sure that the pragma properly overrides it.)

[Sunil_Srivastava]

Hi Richard,

Good point about that extra test. I suppose I need another LGTM for the new test.

No other changes.

[Sunil_Srivastava]

Hi Richard,

Good point about that extra test. I suppose I need another LGTM for the new test.

No other changes.
(Sorry I had missed the code change in the last round)

[rsmith]

If a patch is LGTM'd and some small change is requested at the same time, that typically means "LGTM once you make the following changes, which I trust you to make with no further pre-commit review". If you'd prefer more pre-commit review, of course, that's fine too.

In any case, this still LGTM, assuming it also includes the functional change itself (which is somehow missing from the latest revision here) =)

lib/Basic/Diagnostic.cpp

void DiagnosticsEngine::setSeverity(diag::kind Diag, diag::Severity Map,
   NewState->setMapping(Diag, Mapping);
   DiagStatePoints.insert(Pos+1, DiagStatePoint(NewState,
                                                FullSourceLoc(Loc, *SourceMgr)));
 }
 
 bool DiagnosticsEngine::setSeverityForGroup(diag::Flavor Flavor,
                                             StringRef Group, diag::Severity Map,
                                             SourceLocation Loc) {
+  // Special handling for pragma clang diagnostic ... "-Weverything"
+  // There is no formal group named "everything", so there has to be a special
+  // case for it.
+  if (Group == "everything") {
+   setSeverityForAll(Flavor, Map, Loc);
+   return false;
+  }
+
   // Get the diagnostics in this group.
   SmallVector<diag::kind, 256> GroupDiags;
   if (Diags->getDiagnosticsInGroup(Flavor, Group, GroupDiags))
     return true;
 
   // Set the mapping.
   for (diag::kind Diag : GroupDiags)
     setSeverity(Diag, Map, Loc);

test/Preprocessor/pragma_diagnostic.c

 
 #pragma GCC diagnostic error 42  // expected-error {{expected string literal in pragma diagnostic}}
 
 #pragma GCC diagnostic error "-Wundef" 42  // expected-warning {{unexpected token in pragma diagnostic}}
 #pragma GCC diagnostic error "invalid-name"  // expected-warning {{pragma diagnostic expected option name (e.g. "-Wundef")}}
 
 #pragma GCC diagnostic error "-Winvalid-name"  // expected-warning {{unknown warning group '-Winvalid-name', ignored}}
 
+
+// Testing pragma clang diagnostic with -Weverything
+void ppo(){} // First test that we do not diagnose on this.
+
+#pragma clang diagnostic warning "-Weverything"
+void ppp(){} // expected-warning {{no previous prototype for function 'ppp'}}
+
+#pragma clang diagnostic ignored "-Weverything" // Reset it.
+void ppq(){}
+
+#pragma clang diagnostic error "-Weverything" // Now set to error
+void ppr(){} // expected-error {{no previous prototype for function 'ppr'}}
+
+#pragma clang diagnostic warning "-Weverything" // This should not be effective
+void pps(){} // expected-error {{no previous prototype for function 'pps'}}

test/Preprocessor/pushable-diagnostics.c

 #pragma clang diagnostic ignored "-Wmultichar"
 
 int b = 'df'; // no warning.
 #pragma clang diagnostic pop
 
 int c = 'df';  // expected-warning{{multi-character character constant}}
 
 #pragma clang diagnostic pop // expected-warning{{pragma diagnostic pop could not pop, no matching push}}
+
+// Test -Weverything
+
+void ppo0(){} // first verify that we do not give anything on this
+#pragma clang diagnostic push // now push
+
+#pragma clang diagnostic warning "-Weverything" 
+void ppr1(){} // expected-warning {{no previous prototype for function 'ppr1'}}
+
+#pragma clang diagnostic push // push again
+#pragma clang diagnostic ignored "-Weverything"  // Set to ignore in this level.
+void pps2(){}
+#pragma clang diagnostic warning "-Weverything"  // Set to warning in this level.
+void ppt2(){} // expected-warning {{no previous prototype for function 'ppt2'}}
+#pragma clang diagnostic error "-Weverything"  // Set to error in this level.
+void ppt3(){} // expected-error {{no previous prototype for function 'ppt3'}}
+#pragma clang diagnostic pop // pop should go back to warning level
+
+void pps1(){} // expected-warning {{no previous prototype for function 'pps1'}}
+
+
+#pragma clang diagnostic pop // Another pop should disble it again
+void ppu(){}
+