[cfe-dev] Pushable #pragma diagnostics

[Louis Gerbarg]

On Tue, Jul 7, 2009 at 5:26 PM, Chris Lattner<clattner at apple.com> wrote:
>
> 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:

Okay, I have incorporated the feedback from the other day. Below are
some quick comments

> * 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;

I assume you mean C array's and not std:tr1:array? In the case of tr1
I just assumed it was off limits for the time being.

In the case of C arrays the reason is DiagMappings are always being
used in the DiagMappingsStack std::vector. In general I find
std::vector<std::vector<unsigned char>> works a log better than trying
to make a std::vector of C arrays, since one would have to effectively
wrap up the C array into a class that STL could cope with anyway. IOW,
I did it because:

std::vector<unsigned char> BlankDiags(diag::DIAG_UPPER_LIMIT/2, 0);
DiagMappingsStack.push_back(BlankDiags);

works and:

unsigned char BlankDiags[diag::DIAG_UPPER_LIMIT/2];
memset(BlankDiags, 0, sizeof(BlankDiags));
DiagMappingsStack.push_back(BlankDiags);

doesn't.

> * Please add doxygen comments for these:
>
> +  void pushMappings();
> +  bool popMappings();

Done.

> * 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();

Yeah, that ended up that way because the original line was to long, I
missed it when I split things up. Done.

> * 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) {}

Done.

> also, please pull this up to be at the top of the class and add a doxygen
> comment.
>
> +private:
> +  const bool ClangMode;

I moved it to the top. Now that it is adjacent to the top where
clangMode is described anyway the explanation in the struct
documentation seems sufficient.

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

Done.

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

Done.

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

Done, separate patch attached. I reorganized the diagnostic section a
bit, and now you can see the horror that is my HTMLFu ;-)

Louis
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pushable-pragmas-docs.patch
Type: application/octet-stream
Size: 3816 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090711/9bb0402e/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pushable-pragmas.patch
Type: application/octet-stream
Size: 9620 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090711/9bb0402e/attachment-0001.obj>