Pushable #pragma diagnostics

Clang Frontend
1

I generally compile code with as many warnings enabled as I can, and -Werror. Clang is great for that since it has a lot more flexible options for handling warnings than gcc. It also adopted gcc's recent diagnostic pragmas, which allow one to embed the warning controls directly around the relevant code.

Obviously, doing that in a way that is compatible with gcc was important, but gcc's pragma is deficient in a few ways. The big one is that it has no sort of push/pop semantics, like the pack pragmas. This impacts me since I have some warnings I want to disable in a small section of a header. Unfortunately it is impossible to set the warnings back to the state they were in before that bit of code. It is not just a header issue, since realistically people want to change command line warnings at various points, but the current pragmas force permanent changes in the diagnostic configuration for the entire rest of that compilation unit, despite the change usually only being relevant for a few lines of code.

With that in mind, I would like to propose an extension to the current pragma to add push/pop functionality (similiar to that of pack). There are a couple of ways that this could be done. The first code be to prefix push onto the existing pragmas:

#pragma clang diagnostic push ignored "-Wmultichar"
char d = 'df'; // no warning.
#pragma clang diagnostic pop

That has the benefit of being concise, but it would mean we could have complicated situations like:

#pragma clang diagnostic push ignored "-Wmultichar"
#pragma clang diagnostic warning "-Wall"
char d = 'df'; // no warning.
#pragma clang diagnostic pop

It is unclear what the correct behavior of the pop should be at that point. Should pop all the way back to the push, should it attempt to regenerate a diagnostic state without the push put with the warning "-Wall" ? We could choose something, but it seems to me like it is not worth it. Another option to make push and pop explicit it commands:

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wmultichar"
char d = 'df'; // no warning.
#pragma clang diagnostic pop

The above is slightly less concise in the normal case, but the behavior of the pragma in the presence of multiple changes is clear. There are some other things that are possible with implicit popping and so forth, but they all seem to have complicated behaviors when they nest, and in this case I am in favor of simple, if slightly verbose.

Implementing that is actually not particularly difficult, I have a patch attached for comment.The patch may require some slight cleanups, but it works. I also have some test cases on my system I can cleanup and send if people like this extension.

I think that I could probably generate some better warnings for misuses of the pragma, though what those warnings are depends on what the exact extensions are, so if people are happy with the my proposed extension I may go through and expand on those.

Louis

pushable_diagnostics.patch (6.19 KB)

2

Looks like a good idea; I'll let Chris comment on it, though, since he
first implemented it.

--- a/lib/Lex/Preprocessor.cpp
+++ b/lib/Lex/Preprocessor.cpp
@@ -120,6 +120,20 @@ Preprocessor::~Preprocessor() {
   delete Callbacks;
}

+bool Preprocessor::popDiagnostics() {
+ if (PreviousDiagnostics.empty())
+ return false;

3

Yeah, that is a whoops on my part. Originally I had a std::vector<Diagnostic *> and I was newing and deleting things as a passed them around. Apparently I made a bit of an embarrassing mistake when I changed that :wink:

Louis

4

Thinking about this a bit more, I think that it would be best to
totally eliminate from the class and make getDiagnostics return the
back() item from the vector.

I also think that supporting setDiagnostics is a difficult interface
to maintain in this situation, but that its only in tree use seems to
be the HTML rewriter saving and maintaining state, which is trivial to
do with the vector in the preprocessor anyway. In other words, I think
that it would make sense to replace:

Diagnostic &getDiagnostics() const { ... }
void setDiagnostics( ... }Diagnostic &D) { ... }

with

Diagnostic &getCurrentDiagnostics() const { ... }
void pushDiagnostics() { ... }
void pushDiagnostics(Diagnostic &D) { ... }
bool popDiagnostics() { ... }

Where the parameterless pushDiagnostics pushes a copy of the current
diagnostics, and the one that takes a parameter allows the caller to
push an explicit Diagnostic. I don't think there are any legitimate
circumstance where it makes sense to replace the entire existing stack
of Diagnostics. This is of course an API change, but in the case of
the HTML rewriter it actually simplifies the code because the caller
no longer has to maintain the state.

Louis

5

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.

Louis

pushable-pragmas.patch (9.54 KB)

6

I generally compile code with as many warnings enabled as I can, and -Werror. Clang is great for that since it has a lot more flexible options for handling warnings than gcc. It also adopted gcc's recent diagnostic pragmas, which allow one to embed the warning controls directly around the relevant code.

Cool, thanks for working on this.

It is unclear what the correct behavior of the pop should be at that point. Should pop all the way back to the push, should it attempt to regenerate a diagnostic state without the push put with the warning "-Wall" ? We could choose something, but it seems to me like it is not worth it. Another option to make push and pop explicit it commands:

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wmultichar"
char d = 'df'; // no warning.
#pragma clang diagnostic pop

I strongly prefer this approach. I'll take a look at your latest patch, thanks for working on this Louis!

-Chris

7

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

8

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 :wink:

Louis

pushable-pragmas-docs.patch (3.73 KB)

pushable-pragmas.patch (9.39 KB)

9

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

Right.

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

Ah, ok.

* 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 :wink:

Totally awesome, applied here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090706/018924.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090706/018925.html

Thanks Louis!

-Chris