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

[Sean Silva]

> I do not think that we'll get measurably worse performance as a result of this change.

Please actually measure.

--Sean Silva

On Fri, Jul 27, 2012 at 4:07 PM, Alexander Kornienko <alexfh at google.com> wrote:
> On Fri, Jul 27, 2012 at 11:40 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>>
>> 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.
>
> I do not think that we'll get measurably worse performance as a result of
> this change. This code only works on macro definitions (does anyone know how
> many of those per translation unit should we expect IRL?), and in most cases
> this code will do no more than allocating a buffer (I can make this a class
> member, so it doesn't get allocated each time) and comparing a spelling of a
> couple of tokens (currently I wouldn't expect many macros which expansion
> start with "[[").
> I can also implement Richard's suggestions to make overhead even less: 1.
> avoid adding callback when my warning isn't turned on; 2. lex the search
> string so that matching is done by comparing tokens and not text. And I'm
> going to run performance tests and find out if my patch really has some
> measurable performance penalty. If it doesn't, do you have any objections
> against callback approach?
>
>> 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.
>
> I'm not sure that we retain contents of #undef'ed and redefined macros. And
> we definitely need this as we run analysis after the whole file is lexed.
>
>
>> 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
>
>
> --
> Best regards,
> Alexander Kornienko
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>