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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
