Page MenuHomePhabricator

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

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
153

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

155

nit: new line between functions, same below

208–212

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.Jul 30 2020, 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.Jul 30 2020, 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.

AlexanderLanin marked an inline comment as done.

Rebased and stored at https://github.com/llvm/llvm-project/compare/9d6d4b07a...AlexanderLanin:readability-braces-around-statements?expand=1 so I have a branch next time I need to rebase :-)

Still looking for someone to approve and to commit this change.

Hey Alex, first thanks for the bug fixing! Of course we are interested in improvements in clang-tidy, but i think noone is actually employed to help out here and the normal reviewers have many, many reviews parallel. Sometimes stuff just slips through :/

I try to pick up review an @njames93 seems not have time (i dont have info, and you can just jump in again!).

Two thoughts are added inline. Did you check the new revision against a big code-base like llvm (with fixit on and compilation + testing afterwards)? How did it go, anything outstanding happened?

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

Why is this assertion removed? In the following lines EndLoc is used for a line-number calculation. Shouldn't EndLoc be valid for that to be correct?

clang-tools-extra/clang-tidy/utils/LexerUtils.h
112–113

If personally think that getUnifiedEndLoc is ok. LexerUtils.h is not super exposed and the name makes the point.

@JonasToth I fully realize that it's voluntary and I do not have a solution. I have the same problem in a different area. Not sure what shall become of some OSS projects when it takes that long to get a patch in. But that's a different discussion.

The patch as shown here generates ~130k braces on llvm, you can see it at https://github.com/llvm/llvm-project/commit/16d3ec868cf00

  • The braces are not reviewed
  • check-all still compiles
  • check-all has the exact same results as before the auto-fix

It's not tested against other codebases, but it had a test suite even before this patch.

I'll get back regarding that removed assert ASAP. I don't remember, so I'll add it again and see what happens.

The patch as shown here generates ~130k braces on llvm, you can see it at https://github.com/llvm/llvm-project/commit/16d3ec868cf00

That diff is showing different behaviour to the test cases. In the test cases

if (...)
  something();

Will get changed to

if (...) {
  something();
}

However in the diff it is inserting a new line before the closing brace

if (...) {
  something();

}

This somewhat seems to decrease readability.
It seems that for an if/else only the new line is inserted in the else branch.

@JonasToth: Cannot add assert. EndLoc is indeed invalid in case BeginLoc is outside the macro and EndLoc would be inside the macro. I've added a comment explaining that.

@njames93: Sorry I generated the diff for github upload incorrectly. Yaml newline format has changed between release 11 and 12. One has to provide the new clang-apply-replacements-binary to run-clang-tidy.
Anyway I've added test cases to check for such newlines to prevent regressions.
Fixed diff (only partial this time, only newlines changed): https://github.com/AlexanderLanin/llvm-project/commit/a6cae954e95

njames93 accepted this revision.Oct 16 2020, 6:43 AM

LGTM

clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp
274–275

nit: Hard code the column numbers.

279

While its entirely possible to fix this using the lexer to get indentation, Perfectly happy to leave this job for clang-format. Besides it is needed to handle how a user wants to wrap braces anyway.

283–284

have you tried using the CHECK_EMPTY Directive?

301–302

nit: Hard code the column numbers.

This revision is now accepted and ready to land.Oct 16 2020, 6:43 AM