[PATCH] D30954: Modules: Simulate diagnostic settings for implicit modules

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM

> - The patch-as-is checks whether pragmas should be demoted to warnings for all AST files, not just implicit modules. I can add a bit of logic to `ReadPragmaDiagnosticMappings` that limits it to `F.Kind == MK_ImplicitModule`, but I'm not sure it's necessary. Maybe it hits when reading PCH files, but no tests fail, and I'm not sure this is worse... thoughts?

For the PCH and preamble cases, `PCHValidator` should check that the diagnostic mappings are the same prior to applying the diagnostic pragmas, so there should never be a case where a warning mapping is upgraded to error/fatal error and wasn't when the PCH was built (or vice versa) except when building with `-fno-validate-pch`. Even in that case, the emergent behavior here seems mostly OK (you imagine what the PCH would have looked like if it were built with the current compilation's warning flags), except...

If you build a PCH that contains `#pragma clang diagnostic warning "-Wfoo"` and then use it from a `-Werror=foo` compilation, it looks like we won't notice that we need to upgrade the warning to an error when replaying the PCH's pragma mappings. This is a corner case of a corner case, though.

> - If ReadDiagState sees a back-reference, it doesn't bother to check whether pragmas should be demoted; it assumes it should match whatever was done with the back-reference. I think this could be exercised with -Werror=X on the command-line and pragmas modifying -WX (first "ignored", then "error", then "warning"). Perhaps I should add a FIXME or a comment, but otherwise I think this is okay to miss...

IIRC we only get backreferences from pragma push/pop (and `CurDiagState`), and I think the push/pop cases will always have the same upgrade behavior (you can't push inside a module and pop outside it, for instance, so the starting state for the push and pop should be consistent).

> It could be a back-reference to CurDiagState, which current gets (de)serialized before the pragma mappings. If we instead (de)serialize CurDiagState last, I think this one goes away. Is there a problem with that?

I don't think so, and putting `CurDiagState` last seems better in general (it keeps the states in something much more like source order). I think I only put it second for convenience (so I didn't need to check whether I'd just read the last state in order to handle it differently).



================
Comment at: clang/include/clang/Basic/DiagnosticIDs.h:119-120
+
+  bool wasUpgradedFromWarning() const { return WasUpgradedFromWarning; }
+  void setUpgradedFromWarning(bool Value) { WasUpgradedFromWarning = Value; }
+
----------------
This could do with a documentation comment. Something like "Whether this mapping attempted to map the diagnostic to a warning but was overruled because the diagnostic was already mapped to an error or fatal error."


https://reviews.llvm.org/D30954