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

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL303551: [clang-tidy] readability-braces-around-statements 
false positive with char… (authored by alexfh).

Changed prior to commit:
  https://reviews.llvm.org/D33354?vs=99576=99756#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D33354

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


Index: 
clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- 
clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ 
clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -54,14 +54,15 @@
 SourceLocation findEndLocation(SourceLocation LastTokenLoc,
const SourceManager ,
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(S))`, but it wouldn't work for nested statements.
 SkipEndWhitespaceAndComments = false;


Index: clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -54,14 +54,15 @@
 SourceLocation findEndLocation(SourceLocation LastTokenLoc,
const SourceManager ,
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(S))`, but it wouldn't work for nested statements.
 SkipEndWhitespaceAndComments = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

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


https://reviews.llvm.org/D33354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG. Thank you for investigating this!


https://reviews.llvm.org/D33354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Florian Gross via Phabricator via cfe-commits]

fgross updated this revision to Diff 99576.
fgross added a comment.

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.


https://reviews.llvm.org/D33354

Files:
  clang-tidy/readability/BracesAroundStatementsCheck.cpp


Index: clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -54,14 +54,15 @@
 SourceLocation findEndLocation(SourceLocation LastTokenLoc,
const SourceManager ,
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(S))`, but it wouldn't work for nested statements.
 SkipEndWhitespaceAndComments = false;


Index: clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -54,14 +54,15 @@
 SourceLocation findEndLocation(SourceLocation LastTokenLoc,
const SourceManager ,
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(S))`, but it wouldn't work for nested statements.
 SkipEndWhitespaceAndComments = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I'll repeat my comment from https://reviews.llvm.org/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.


https://reviews.llvm.org/D33354



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Florian Gross via Phabricator via cfe-commits]

fgross created this revision.
Herald added a subscriber: xazax.hun.

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.


https://reviews.llvm.org/D33354

Files:
  clang-tidy/readability/BracesAroundStatementsCheck.cpp
  include/clang/Basic/TokenKinds.h
  test/clang-tidy/readability-braces-around-statements-single-line.cpp


Index: include/clang/Basic/TokenKinds.h
===
--- include/clang/Basic/TokenKinds.h
+++ include/clang/Basic/TokenKinds.h
@@ -82,12 +82,17 @@
  K == tok::utf32_string_literal;
 }
 
+/// \brief Return true if this is a char constant token
+inline bool isCharConstant(TokenKind K) {
+  return K == tok::char_constant || K == tok::wide_char_constant ||
+ K == tok::utf8_char_constant || K == tok::utf16_char_constant ||
+ K == tok::utf32_char_constant;
+}
+
 /// \brief Return true if this is a "literal" kind, like a numeric
 /// constant, string, etc.
 inline bool isLiteral(TokenKind K) {
-  return K == tok::numeric_constant || K == tok::char_constant ||
- K == tok::wide_char_constant || K == tok::utf8_char_constant ||
- K == tok::utf16_char_constant || K == tok::utf32_char_constant ||
+  return K == tok::numeric_constant || isCharConstant(K) ||
  isStringLiteral(K) || K == tok::angle_string_literal;
 }
 
Index: test/clang-tidy/readability-braces-around-statements-single-line.cpp
===
--- test/clang-tidy/readability-braces-around-statements-single-line.cpp
+++ test/clang-tidy/readability-braces-around-statements-single-line.cpp
@@ -31,3 +31,21 @@
   // CHECK-FIXES: if (cond("if4") /*comment*/) {
   // CHECK-FIXES: }
 }
+
+const char *test2() {
+  if (cond("if1"))
+return "string";
+
+
+}
+
+char test3() {
+  if (cond("if1"))
+return 'a';
+
+
+  if (cond("if1"))
+return (char)L'a';
+
+
+}
Index: clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -61,7 +61,8 @@
   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 || isCharConstant(TokKind) ||
+  isStringLiteral(TokKind)) {
 // If we are at ";" or "}", we found the last token. We could use as well
 // `if (isa(S))`, but it wouldn't work for nested statements.
 SkipEndWhitespaceAndComments = false;


Index: include/clang/Basic/TokenKinds.h
===
--- include/clang/Basic/TokenKinds.h
+++ include/clang/Basic/TokenKinds.h
@@ -82,12 +82,17 @@
  K == tok::utf32_string_literal;
 }
 
+/// \brief Return true if this is a char constant token
+inline bool isCharConstant(TokenKind K) {
+  return K == tok::char_constant || K == tok::wide_char_constant ||
+ K == tok::utf8_char_constant || K == tok::utf16_char_constant ||
+ K == tok::utf32_char_constant;
+}
+
 /// \brief Return true if this is a "literal" kind, like a numeric
 /// constant, string, etc.
 inline bool isLiteral(TokenKind K) {
-  return K == tok::numeric_constant || K == tok::char_constant ||
- K == tok::wide_char_constant || K == tok::utf8_char_constant ||
- K == tok::utf16_char_constant || K == tok::utf32_char_constant ||
+  return K == tok::numeric_constant || isCharConstant(K) ||
  isStringLiteral(K) || K == tok::angle_string_literal;
 }
 
Index: test/clang-tidy/readability-braces-around-statements-single-line.cpp
===
--- test/clang-tidy/readability-braces-around-statements-single-line.cpp
+++ test/clang-tidy/readability-braces-around-statements-single-line.cpp
@@ -31,3 +31,21 @@
   // CHECK-FIXES: if (cond("if4") /*comment*/) {
   // CHECK-FIXES: }
 }
+
+const char *test2() {
+  if (cond("if1"))
+return "string";
+
+
+}
+
+char test3() {
+  if (cond("if1"))
+return 'a';
+
+
+  if (cond("if1"))
+return (char)L'a';
+
+
+}
Index: clang-tidy/readability/BracesAroundStatementsCheck.cpp
===
--- clang-tidy/readability/BracesAroundStatementsCheck.cpp
+++ clang-tidy/readability/BracesAroundStatementsCheck.cpp
@@ -61,7