[clang-tidy] readability-braces-around-statements false positive with char literals

[fgross]

Summary

Single-line if statements cause a false positive when the last token in the conditional statement is a char constant:

if (condition)
  return 'a';

For some reason findEndLocation seems to skips too many (vertical) whitespaces in this case. The same problem already occured with string literals (https://reviews.llvm.org/D25558), and was fixed by adding a special check for this very case. I just extended the condition to also include char constants. No idea what really causes the issue though.

[alexfh]

I'll repeat my comment from D25558: "I'm thoroughly confused as to why the code in your test was not handled correctly and why this is the right fix. Can you explain?" The "For some reason ..." part doesn't really explain anything. I guess, we're papering over a more generic problem, and someone has to figure out what it is and how to fix it properly.

[fgross]

After some digging into it, here is my uneducated guess:

The comment in findEndLocation states that "Loc points to the beginning of the last token before ';'". But checkStmt calls it with

FileRange.getEnd().getLocWithOffset(-1)

so in fact it points to the last char of the last token. For a string literal this would be '"' or ''', not enough for Lexer::getLocForEndOfToken to query the correct token type. It ends up moving behind the following ';' and skipping all whitespaces to next token.

I've updated the diff, this seems to resolve the issue. But I'm sure there is a way to pass the correct location in the first place.

[alexfh]

LG. Thank you for investigating this!

[alexfh]

I'm going to commit the patch for you, but I guess, you might want to ask for commit access (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).

[fgross]

Thanks a lot, will do.

clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp

for (;;) {
     // Fast-forward current token.
     Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
   }
 }
 
 SourceLocation findEndLocation(SourceLocation LastTokenLoc,
                                const SourceManager &SM,
                                const ASTContext *Context) {
-  SourceLocation Loc = LastTokenLoc;
+  SourceLocation Loc =
+      Lexer::GetBeginningOfToken(LastTokenLoc, SM, Context->getLangOpts());
   // Loc points to the beginning of the last (non-comment non-ws) token
   // before end or ';'.
   assert(Loc.isValid());
   bool SkipEndWhitespaceAndComments = true;
   tok::TokenKind TokKind = getTokenKind(Loc, SM, Context);
   if (TokKind == tok::NUM_TOKENS || TokKind == tok::semi ||
-      TokKind == tok::r_brace || isStringLiteral(TokKind)) {
+      TokKind == tok::r_brace) {
     // If we are at ";" or "}", we found the last token. We could use as well
     // `if (isa<NullStmt>(S))`, but it wouldn't work for nested statements.
     SkipEndWhitespaceAndComments = false;
   }
 
   Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, Context->getLangOpts());
   // Loc points past the last token before end or after ';'.
   if (SkipEndWhitespaceAndComments) {