Hi Alex,<div><br></div><div>Please don't add the PP callback if the fallthrough warning isn't enabled. Also, have you made any performance measurements for this change?<br><br>I would prefer that the CompatibilityMacroFinder were given a list of token kinds and identifier infos to look for rather than a string. That would probably improve the performance a bit, and would allow your diagnostic to work in cases where the tokens don't have the obvious spelling (for instance, "<:<:clang::fallthrough:>:>").<br>
<br><div class="gmail_quote">On Tue, Jul 24, 2012 at 10:11 AM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This patch adds an automatic detection of a compatibility macros used in specific projects to hide constructions based on non-portable features (e.g. custom C++11 attributes). It helps to adapt diagnostic messages and fix-it hints to use these compatibility macros instead of the actual construct.<div>
<div><br></div><div>This feature is implemented in AnalysisBasedWarnings.cpp, as there's currently only one diagnostic which gets profit from this - diagnostic of unannotated fall-through between switch labels. But the code of the CompatibilityMacroFinder class was intentionally made reasonably generic, and doesn't contain any specific bindings to this diagnostic. The class is a lightweight handler of PPCallbacks::MacroDefined calls. An instance of it is registered via Preprocessor::addPPCallbacks for each token sequence (specified in plain text) to find in macros (currently only one). It keeps all macros with the specified expansion token sequence and then can determine which one can be used instead of the actual construct in a specific code location.</div>
<div><br></div><div>A motivating example for this feature:</div><div>There's the <font face="courier new, monospace">-Wimplicit-fallthrough</font> warning, which detects <font face="courier new, monospace">[[clang::fallthrough]];</font> construct as an annotation of an intended fall-through. In projects which should be compiled both in C++11 mode and in C++03 mode, this construct can not be used as is, so it should be wrapped in a macro, e.g.:</div>
</div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div><div><div><font face="courier new, monospace" size="1">#ifdef __clang__</font></div></div></div><div><div><div><font face="courier new, monospace" size="1">#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")</font></div>
</div></div><div><div><div><font face="courier new, monospace" size="1">#define LLVM_FALLTHROUGH [[clang::fallthrough]]</font></div></div></div><div><div><div><font face="courier new, monospace" size="1">#endif</font></div>
</div></div><div><div><div><font face="courier new, monospace" size="1">#endif</font></div></div></div><div><div><div><font face="courier new, monospace" size="1"><br></font></div></div></div><div><div><div><font face="courier new, monospace" size="1">#ifndef LLVM_FALLTHROUGH</font></div>
</div></div><div><div><div><font face="courier new, monospace" size="1">#define LLVM_FALLTHROUGH do { } while (0)</font></div></div></div><div><div><div><font face="courier new, monospace" size="1">#endif</font></div></div>
</div></blockquote><div><div><br></div><div>Prior to this feature diagnostic messages would say:</div><div><div><font face="courier new, monospace" size="1">test.cpp:156:5: <b>warning: unannotated fall-through between switch labels</b></font></div>
<div><font face="courier new, monospace" size="1"> case 223:</font></div><div><font face="courier new, monospace" size="1"> ^</font></div><div><font face="courier new, monospace" size="1">test.cpp:156:5: <b>note: insert '[[clang::fallthrough]];' to silence this warning</b></font></div>
<div><font face="courier new, monospace" size="1"> case 223:</font></div><div><font face="courier new, monospace" size="1"> ^</font></div><div><font face="courier new, monospace" size="1"> <b>[[clang::fallthrough]];</b> </font></div>
<div><font face="courier new, monospace" size="1">test.cpp:156:5: <b>note: insert 'break;' to avoid fall-through</b></font></div><div><font face="courier new, monospace" size="1"> case 223:</font></div><div><font face="courier new, monospace" size="1"> ^</font></div>
<div><font face="courier new, monospace" size="1"> break;</font></div></div><div><br></div><div>Applying the first of these fix-it hints will lead to broken builds in C++03 mode, which is usually not desired.</div><div>
<br></div><div>But with the automatic detection feature the diagnostic will suggest the correct compatibility macro available in the corresponding source location:</div><div><div><span style="font-family:'courier new',monospace;font-size:x-small">...</span></div>
<div><span style="font-family:'courier new',monospace;font-size:x-small">test.cpp:156:5:</span><span style="font-family:'courier new',monospace;font-size:x-small"> </span><b style="font-family:'courier new',monospace;font-size:x-small">note: insert 'LLVM_FALLTHROUGH;' to silence this warning</b><br>
</div><div><font face="courier new, monospace" size="1"> case 223:</font></div><div><font face="courier new, monospace" size="1"> ^</font></div><div><font face="courier new, monospace" size="1"> </font><b style="font-family:'courier new',monospace;font-size:x-small">LLVM_FALLTHROUGH; </b></div>
<div><font face="courier new, monospace" size="1">...</font></div></div><div><div><br></div><div>Please, review this patch. Thank you!</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- <br>
<div>Best regards,</div><div>Alexander Kornienko</div></div></font></span></div></div>
</blockquote></div><br></div>