Page MenuHomePhabricator

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

Authored by fgross on May 19 2017, 6:01 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

fgross created this revision.May 19 2017, 6:01 AM
alexfh edited edge metadata.May 19 2017, 8:35 AM

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 updated this revision to Diff 99576.May 19 2017, 9:35 AM

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 accepted this revision.May 22 2017, 5:33 AM

LG. Thank you for investigating this!

This revision is now accepted and ready to land.May 22 2017, 5:33 AM

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).

This revision was automatically updated to reflect the committed changes.

Thanks a lot, will do.