[cfe-dev] Pushable #pragma diagnostics

[Chris Lattner]

On Jul 7, 2009, at 10:22 AM, Louis Gerbarg wrote:

> Okay, here is a cleaned up version of the patch. I ended up moving the
> push/pop into the Diagnostic class (just pushing and popping the
> DiagMappings), since pushing the entire Diagnostic objects resulted in
> issues with losing DiagClients. This patch includes some new test
> cases for the feature, and passes all existing test cases.

Some specific comments about the patch:

* Why make DiagMappings be a vector instead of an array here?
-  mutable unsigned char DiagMappings[diag::DIAG_UPPER_LIMIT/2];
-
+
+  typedef std::vector<unsigned char> DiagMappings;
+  mutable std::vector<DiagMappings> DiagMappingsStack;


* Please add doxygen comments for these:

+  void pushMappings();
+  bool popMappings();


* This code:
    unsigned getDiagnosticMappingInfo(diag::kind Diag) const {
-    return (diag::Mapping)((DiagMappings[Diag/2] >> (Diag & 1)*4) &  
15);
+    DiagMappings currentMappings = DiagMappingsStack.back();
+    return (diag::Mapping)((currentMappings[Diag/2] >> (Diag & 1)*4)  
& 15);
    }

Copies the vector, you probably want:
   const DiagMappings &currentMappings = DiagMappingsStack.back();


* Please expand the doxygen comment on this class to describe "clang  
mode":
  /// PragmaDiagnosticHandler - e.g. '#pragma GCC diagnostic ignored "- 
Wformat"'
  struct PragmaDiagnosticHandler : public PragmaHandler {
-  PragmaDiagnosticHandler(const IdentifierInfo *ID) :  
PragmaHandler(ID) {}
+  PragmaDiagnosticHandler(const IdentifierInfo *ID,
+                          const bool clangMode) : PragmaHandler(ID),
+                                                   
ClangMode(clangMode) {}

also, please pull this up to be at the top of the class and add a  
doxygen comment.

+private:
+  const bool ClangMode;




* Some minor coding style stuff:

+      if (II->isStr("pop")) {
+        if(!PP.getDiagnostics().popMappings()) {
+          PP.Diag(Tok, diag::warn_pragma_diagnostic_clang_cannot_ppp);
+        }
+        return;
+      } else if (II->isStr("push")) {

Please format this like:

+      if (II->isStr("pop")) {
+        if (!PP.getDiagnostics().popMappings())
+          PP.Diag(Tok, diag::warn_pragma_diagnostic_clang_cannot_ppp);
+        return;
+      }
+
+      if (II->isStr("push")) {

space before the if, avoid nested indent of "else if" since you have  
an early return.



In this:
+      if (ClangMode) {
+        PP.Diag(Tok, diag::warn_pragma_diagnostic_clang_invalid);
+      } else {
+        PP.Diag(Tok, diag::warn_pragma_diagnostic_gcc_invalid);

And a couple other places it would be better to do:
    unsigned Diag = ClangMode ?  diag::warn_pr... : diag::...;
    PP.Diag(Tok, Diag);

* Mega bonus points for describing how the existing and new stuff  
works in docs/UserManual.html#diagnostics

Overall, this looks like a great approach, thanks for working on this  
Louis!

-Chris