[cfe-commits] [PATCH] Compatibility macro detection for the -Wimplicit-fallthrough diagnostic.

[Richard Smith]

================
Comment at: tools/clang/lib/Sema/AnalysisBasedWarnings.cpp:687
@@ +686,3 @@
+       it != MI->tokens_end(); ++it) {
+    StringRef S = PP.getSpelling(*it, Buf);
+    if (StringRef(P, S.size()) != S)
----------------
Alexander Kornienko wrote:
> Richard Smith wrote:
> > Alexander Kornienko wrote:
> > > If this seems to be not efficient, does anyone have an idea of how to conveniently create a Token array for a known character sequence (run Lexer? fill by hand? both ways seem to be too verbose).
> > > And after that, how to compare two token sequences? Manually compare getKind() and for certain kinds getIdentifierInfo(), getLiteralData(), getAnnotationValue() and some flags?
> > I would recommend that you pass in a list of token values to look for, where a token value is a TokenKind and (for a tok_identifier) an IdentifierInfo*, rather than passing in a const char* and comparing it against the macro's spelling. That would be faster, and would also catch cases where an alternate spelling was used for the '[' or ']' tokens.
> This only happens when we issue this specific diagnostic, so it's unlikely to become a real performance problem.
> 
> What I don't like in the approach you suggested, is that in order to make it more generic we'd have to handle not only identifiers, but at least literals (and maybe more?). And it is much more verbose.
> 
> Or you still think it's still better?
I still think it's better to test the sequence of tokens themselves rather than forming a string from them and testing that. As another example of things that can go wrong with your current approach, it matches (note the space):

#define BROKEN_FALLTHROUGH_MACRO [[clang::fall through]]

I would suggest that you don't worry about matching literals until/unless you have a use case for that.

================
Comment at: tools/clang/lib/Lex/MacroInfo.cpp:64
@@ +63,3 @@
+  for (const MacroInfo *MI = this; MI; MI = MI->PreviousDefinition) {
+    if (!SM.getFileEntryForID(SM.getFileID(MI->Location)) ||
+        SM.isBeforeInTranslationUnit(MI->Location, L))
----------------
Alexander Kornienko wrote:
> Richard Smith wrote:
> > Under what circumstances is this test necessary?
> It's intended to handle a case when a macro is defined via command line. Does isBeforeInTranslationUnit handle this case?
Yes, isBeforeInTranslationUnit should handle that.


http://llvm-reviews.chandlerc.com/D50