Page MenuHomePhabricator

[clang-tidy] fix readability-braces-around-statements Stmt type dependency
Needs ReviewPublic

Authored by AlexanderLanin on Mar 7 2020, 10:21 AM.

Details

Summary

Replaces Token based approach to identify EndLoc of Stmt with AST traversal.
This also improves handling of macros.

Fixes Bugs 22785, 25970 and 35754.

Diff Detail

Event Timeline

AlexanderLanin created this revision.Mar 7 2020, 10:21 AM

Oh sorry. It's crashing. I'll look into it.

AlexanderLanin edited the summary of this revision. (Show Details)
AlexanderLanin added projects: Restricted Project, Restricted Project.
  1. Extended Test for another macro scenario.
  2. Added graceful termination to handle problematic locations.
  3. Changed Lexer::findNextToken to no longer run into that graceful termination. Test is passing. I'm not sure whether that's a good idea? check-clang and check-clang-tools are passing with this change.
AlexanderLanin marked an inline comment as done.Mar 11 2020, 1:20 PM
AlexanderLanin added inline comments.
clang-tools-extra/clang-tidy/utils/LexerUtils.h
112–113

Maybe this should be more in the line of getStmtLikeEndLoc?
With a note "This will fail horribly if Stmt is not Stmt-like" - whatever that means ;-)

Compare http://lists.llvm.org/pipermail/cfe-dev/2020-March/064844.html

Here is the rest of the mail thread: http://lists.llvm.org/pipermail/cfe-dev/2020-February/064743.html

AlexanderLanin edited the summary of this revision. (Show Details)
  • Added GotoStmt and SEHLeaveStmt as learned from D76108.
  • After applying D76037 and D76054 I was able to run-clang-tidy on llvm and therefore found some new macro related issues

Accidentally reverted test file renaming in last commit. Fixed now.
Rebased on current master.

AlexanderLanin marked an inline comment as not done.Mar 17 2020, 1:36 AM
AlexanderLanin added inline comments.
clang-tools-extra/clang-tidy/utils/LexerUtils.h
112–113

Sorry this marked itself as Done.
I'd still like feedback on the name.
getStmtLikeEndLoc? getEndLocInclSemi?

AlexanderLanin marked an inline comment as not done.
AlexanderLanin edited the summary of this revision. (Show Details)

Add further tests from more bug reports.

Can you add a test case when the else body ends with braces. In one of the bug reports that case was mentioned

else
  X = {6};
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
86

As the old behaviour didn't use check fixes next is hard to tell how the semi colon was handled for null stmt. In the old version were they kept when brackets were added, or removed. If they were removed I'd prefer that behaviour to stay. If you just made the test more explicit, disregard this.

AlexanderLanin marked 2 inline comments as done.Mar 19 2020, 12:32 PM
AlexanderLanin added inline comments.
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
86

It's not new behavior. Although now that you mention it I'm not sure it's good behavior. However I have not modified it.

I found that some of the tests here did not actually test what they were supposed to. For example this test was matching against 'do_something("for");' instead of the NullStmt input. That's why I modified several either with unique strings or with -NEXT.

AlexanderLanin marked an inline comment as done.

new test case as requested (line 256)

njames93 added inline comments.Mar 19 2020, 1:28 PM
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
86

Fair enough. I think the way NullStmt is handled by this check isn't great. But I don't know what the preferred way would be?

  • No braces added
  • Braces add and semi-colon removed
  • Braces added, semi-colon kept
  • Just add an option to let users specify behaviour

In any case it's out of scope for this revision.

*ping*

I have an opinion on NullStmt, but I would rather see this merged ;-)
Behavior is same as before.

jdenny added a subscriber: jdenny.Apr 25 2020, 7:17 AM

*ping*
Sorry, but I have to ask: Is there no interest in improving clang-tidy?
Don't get me wrong. I know it's mostly volunteered time. That's also why this ping took almost 2 months for me.
I frankly lost interest in contributing as I have trouble getting any patches included - and I don't have that much free time anymore. Obviously the time issue is more important, but the other one is still a problem.

*ping*
Sorry, but I have to ask: Is there no interest in improving clang-tidy?
Don't get me wrong. I know it's mostly volunteered time. That's also why this ping took almost 2 months for me.
I frankly lost interest in contributing as I have trouble getting any patches included - and I don't have that much free time anymore. Obviously the time issue is more important, but the other one is still a problem.

I agree this needs fixing too, Added a few more reviewers to help move this along

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
154

llvm::isa can take multiple types to check for any matching. Same goes for below

156

nit: new line between functions, same below

209–213

nit: remove braces and empty line

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
99

Can you explicitly spell the column rather than just regexing for any number, helps prevent any changes down the line altering where the warning is displayed by accident.

213

Does this line need to be here?

Addressed findings by @njames93

AlexanderLanin marked 5 inline comments as done.Thu, Jul 30, 3:24 PM
AlexanderLanin added inline comments.
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
99

"where the warning is displayed by accident"... well it did check the line number. But I've extended with column number as requested.

213

Yes and no.
You are correct in that it doesn't check for any change. Except maybe that no second brace is added.
However what it does is ensuring that the following CHECK-FIXES is applied after cond("break and continue"). This can surely be solved differently, but currently the next CHECK-FIXES is a rather generic if(true) which could be anywhere in code. This plus the // CHECK-FIXES: } //end sets the scope for the CHECK-FIXES in between. The // end comment is there to ensure that not some other random } is matched.

njames93 added inline comments.Thu, Jul 30, 3:57 PM
clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
213

Ahh fair enough.
However I don't think it will check to ensure a second brace wasn't added unless you use an eol anchor on the line {{$}} due to how FileCheck works.