[cfe-dev] [PATCH] Automatic detection of compatibility macros for non-portable diagnostics

[Chandler Carruth]

FWIW, at a high level I find the callback approach is very worrisome.

The callback system was designed to be fast when the callbacks were
completely null, and impose a non-trivial cost on the preprocessor
otherwise.

Can we not walk back through the macro definitions after-the-fact? I would
expect the preprocessor to still have all the information we need.


On Fri, Jul 27, 2012 at 2:36 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> Hi Alex,
>
> Please don't add the PP callback if the fallthrough warning isn't enabled.
> Also, have you made any performance measurements for this change?
>
> 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:>:>").
>
>
> On Tue, Jul 24, 2012 at 10:11 AM, Alexander Kornienko <alexfh at google.com>wrote:
>
>> 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.
>>
>> 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.
>>
>> A motivating example for this feature:
>> There's the -Wimplicit-fallthrough warning, which detects
>> [[clang::fallthrough]]; 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.:
>>
>> #ifdef __clang__
>> #if __has_feature(cxx_attributes) &&
>> __has_warning("-Wimplicit-fallthrough")
>> #define LLVM_FALLTHROUGH [[clang::fallthrough]]
>> #endif
>> #endif
>>
>> #ifndef LLVM_FALLTHROUGH
>> #define LLVM_FALLTHROUGH do { } while (0)
>> #endif
>>
>>
>> Prior to this feature diagnostic messages would say:
>> test.cpp:156:5: *warning: unannotated fall-through between switch labels*
>>     case 223:
>>     ^
>> test.cpp:156:5: *note: insert '[[clang::fallthrough]];' to silence this
>> warning*
>>     case 223:
>>     ^
>>     *[[clang::fallthrough]];*
>> test.cpp:156:5: *note: insert 'break;' to avoid fall-through*
>>     case 223:
>>     ^
>>     break;
>>
>> Applying the first of these fix-it hints will lead to broken builds in
>> C++03 mode, which is usually not desired.
>>
>> But with the automatic detection feature the diagnostic will suggest the
>> correct compatibility macro available in the corresponding source location:
>> ...
>> test.cpp:156:5: *note: insert 'LLVM_FALLTHROUGH;' to silence this warning
>> *
>>     case 223:
>>     ^
>>     *LLVM_FALLTHROUGH; *
>> ...
>>
>> Please, review this patch. Thank you!
>>
>> --
>> Best regards,
>> Alexander Kornienko
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120727/5be401b4/attachment.html>