This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Sunil_Srivastava on Nov 30 2015, 3:51 PM.

Details

Summary

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

Diff Detail

Event Timeline

Sunil_Srivastava retitled this revision from to Accept "-Weverything" in pragma clang diagnostic ....
Sunil_Srivastava updated this object.
Sunil_Srivastava added a subscriber: cfe-commits.
rsmith added a subscriber: rsmith.Nov 30 2015, 7:12 PM
rsmith added inline comments.
lib/Basic/Diagnostic.cpp
251–257 ↗(On Diff #41448)

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.

Please see the comment about getAllDiagnostics

lib/Basic/Diagnostic.cpp
251–257 ↗(On Diff #41448)

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 added a reviewer: rsmith.

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

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 edited edge metadata.Feb 10 2016, 1:38 PM

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 edited edge metadata.

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 accepted this revision.Feb 11 2016, 2:09 PM
rsmith edited edge metadata.

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.)

This revision is now accepted and ready to land.Feb 11 2016, 2:09 PM
Sunil_Srivastava edited edge metadata.

Hi Richard,

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

No other changes.

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)

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) =)

This revision was automatically updated to reflect the committed changes.