Page MenuHomePhabricator

Misleading Indentation check
ClosedPublic

Authored by xazax.hun on Apr 27 2016, 5:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nlewycky added inline comments.Apr 27 2016, 1:16 PM
clang-tidy/readability/MisleadingIndentationCheck.cpp
31 ↗(On Diff #55247)

Typo of "potential" as "potentional".

56 ↗(On Diff #55247)

Typo of "indented" as "indentated".

Pajesz updated this revision to Diff 55389.Apr 28 2016, 2:38 AM
Pajesz edited edge metadata.
Pajesz marked 5 inline comments as done.

Both dangling else and the other check now ignore one-line if-else statements. Corrected other reviews as well.

Both dangling else and the other check now ignore one-line if-else statements. Corrected other reviews as well.

I can not see a test case for one line if-then-else. Could you add it?

etienneb requested changes to this revision.Apr 29 2016, 10:27 AM
etienneb edited edge metadata.

Could you lift some logics to the matcher instead of the check.

clang-tidy/readability/MisleadingIndentationCheck.cpp
22 ↗(On Diff #55389)

nit: const

40 ↗(On Diff #55389)

nit: const

67 ↗(On Diff #55389)

of if statement -> previous if statement

72 ↗(On Diff #55389)

instead of doing check at line 23:

if (!If->getElse())

you should use 'hasElse' matcher here.

As I get it, you want to match something like:

anyOf(
  ifStatement(hasThen(stmt().bind("last")),
                     hasElse(unless(compoundStmt())),
  ifStatement(hasThen(unless(compoundStmt())),
                     unless(hasElse(stmt().bind("last")))
)

Then, by getting node named "last" you can retrieve the indentation of the last statement.

77 ↗(On Diff #55389)

nit: remove empty line

clang-tidy/readability/MisleadingIndentationCheck.h
30 ↗(On Diff #55389)

nit: private

docs/clang-tidy/checks/readability-misleading-indentation.rst
8 ↗(On Diff #55389)

could you fix these long lines.

test/clang-tidy/readability-misleading-indentation.cpp
62 ↗(On Diff #55389)

I would like to see more tests with { }

if (xxx) {
} else {
}
if (xxx) {
}
else {
}
if (xxx)
{
}
else
{
}
if (xxx)
  {
  }
else
  {
  }
62 ↗(On Diff #55389)

could you add test with macro:

#define BLOCK
  if (xxx)    \
    stm1();  \
    stm2();
This revision now requires changes to proceed.Apr 29 2016, 10:27 AM

You should not limit the checker to "if".
The for-statement and while-statement are also wrong for the same reasons:

while (x)
  statement1();
  statement2();
for (...)
  statement1();
  statement2();

You should test your code over large code base.
You should catch these cases:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/frame/FrameView.cpp&q=setMediaType&sq=package:chromium&type=cs&l=1253

You will also find strange cases like:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/source/i18n/usearch.cpp&q=initializepattern&sq=package:chromium&type=cs&l=438

The rule also apply for statements in a same compound:

{
  statement1();
  statement2();
    statement3();

see: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/source/common/ushape.cpp&q=third_party/icu/source/common/ushape.cpp&sq=package:chromium&type=cs&l=784

But this can be a further improvement.

The rule also apply for statements in a same compound:

{
  statement1();
  statement2();
    statement3();

But this can be a further improvement.

I believe this might be an intentional omission, since this can not imply semantic problems. But I agree that, this addition makes sense.

The rule also apply for statements in a same compound:

{
  statement1();
  statement2();
    statement3();

But this can be a further improvement.

I believe this might be an intentional omission, since this can not imply semantic problems. But I agree that, this addition makes sense.

Fair enough. This checker is in 'readbility', so I don't see why not.
But, feel free to postpone. Or let someone else take it.

Pajesz updated this revision to Diff 61651.Jun 23 2016, 2:19 AM
Pajesz edited edge metadata.

Checker now works with for and while statements as well, new tests were added, other syntactical and logical updates have been made.

alexfh added a comment.Jul 6 2016, 7:10 AM

Gergely, it seems that the last diff is missing clang-tidy/readability/MisleadingIndentationCheck.cpp. A few more comments below.

docs/clang-tidy/checks/readability-misleading-indentation.rst
13 ↗(On Diff #61651)

nit: Please use double backquotes to mark inline code fragments.

test/clang-tidy/readability-misleading-indentation.cpp
16 ↗(On Diff #61651)
  1. This is not a part of the previous CHECK-MESSAGES. Is it intended? It's fine to have long lines in tests.
  2. Code snippets in the message should be enclosed in single quotes ('else', 'if(cond2)' ...).
  3. Please specify each unique message once completely (including the [check-name] part).
17 ↗(On Diff #61651)

These CHECK-FIXES: are totally unreliable, since they are all matching the same pattern. FileCheck (which is used to verify the output against these CHECK lines) maintains no implicit relation between the check line location and the line number matched in the output. The two things that matter to FileCheck are the order of the matched lines and their content. So the patterns need to be unique to avoid matches to incorrect lines. Change them to, e.g. // comment1, // comment2, ...

63 ↗(On Diff #61651)

What about this comment?

63 ↗(On Diff #61651)

What about this comment?

alexfh requested changes to this revision.Jul 6 2016, 7:11 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Jul 6 2016, 7:11 AM
Pajesz updated this revision to Diff 63925.Jul 14 2016, 12:03 AM
Pajesz edited edge metadata.

Minor changes in tests and doc and diff should be full now.

Please mark all addressed comments "Done".

test/clang-tidy/readability-misleading-indentation.cpp
15 ↗(On Diff #63925)

Please place // CHECK comments at column 1 or 3. Indenting them like this makes the test less readable.

16 ↗(On Diff #63925)

Did you run the tests after changes? I don't think the // comment1 line can actually appear in the fixed code, when it's not there before the fix.

alexfh requested changes to this revision.Jul 19 2016, 7:03 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Jul 19 2016, 7:03 AM
chh added a subscriber: chh.Jul 26 2016, 3:53 PM
Pajesz marked 12 inline comments as done.Sep 5 2016, 11:14 AM

so i have to submit K good to know

test/clang-tidy/readability-misleading-indentation.cpp
48 ↗(On Diff #55247)

Since brackets are used I don't think we should warn about it. It would indeed cause man false positives.

16 ↗(On Diff #63925)

Could you please explain what check-fixes does and how? Im not sure about it.

63 ↗(On Diff #61651)

The checker doesn't work for macro's I don't know why and how to fix it so I just ignored it.

dkrupp added a subscriber: dkrupp.Sep 13 2016, 9:59 AM
xazax.hun added inline comments.Sep 27 2016, 3:52 AM
test/clang-tidy/readability-misleading-indentation.cpp
16 ↗(On Diff #63925)

You can read the documentation here: http://clang.llvm.org/extra/clang-tidy/#testing-checks

Didn't you execute the tests? (Using make check-all or some similar command)

The CHECK-FIXES line will check the content of the file after the fixits are applied. The check will fail when the specified pattern is not found in the result.

Pajesz requested a review of this revision.Oct 4 2016, 4:59 AM
Pajesz marked an inline comment as done.

Hello!

Gonna fix the tests ASAP! Any other suggestions, fixes, improvements considering the checker?

danielmarjamaki added inline comments.Oct 5 2016, 6:18 AM
clang-tidy/readability/MisleadingIndentationCheck.cpp
20 ↗(On Diff #63925)

There is no handling of tabs and spaces by danglingElseCheck as far as I see.

The "if" might for example be indented with spaces. And then the corresponding "else" is indented with a tab. Then I guess there is false positive.

If the "if" is indented with 2 spaces and the "else" is indented with 2 tabs. then I assume there is false negative.

It's unfortunate. Is it ok?

34 ↗(On Diff #63925)

I see no test case that says "potential dangling else". If I'm not mistaken that should be added.

Is it really sufficient to write that? It's not obvious to me why clang-tidy would think it's dangling.

44 ↗(On Diff #63925)

You don't need to use both isa and dyn_cast:

if (const auto *CurrentIf = dyn_cast<IfStmt>(CurrentStmt)) {
    Inside = CurrentIf->getElse() ? CurrentIf->getElse() : CurrentIf->getThen();
} ....
clang-tidy/readability/MisleadingIndentationCheck.h
20 ↗(On Diff #63925)

there are too many spaces in this comment

alexfh requested changes to this revision.Nov 30 2016, 8:36 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Nov 30 2016, 8:36 AM
xazax.hun commandeered this revision.Feb 13 2017, 5:30 AM
xazax.hun edited reviewers, added: Pajesz; removed: xazax.hun.
xazax.hun edited edge metadata.

The original author is no longer available.

xazax.hun updated this revision to Diff 88182.Feb 13 2017, 5:31 AM
xazax.hun edited edge metadata.
  • Updated to latest trunk.
  • Mentioned check in the release notes.
  • Documented the limitation that tabs and spaces need to be consistent for this check to work.
  • Fixed (hopefully all) review comments.
  • Fixed the test cases.
  • Minor code cleanups.
xazax.hun marked 13 inline comments as done.Feb 13 2017, 5:34 AM
xazax.hun added inline comments.
clang-tidy/readability/MisleadingIndentationCheck.cpp
20 ↗(On Diff #63925)

Documented this limitation.

alexfh requested changes to this revision.Feb 13 2017, 8:11 AM
alexfh added inline comments.
clang-tidy/readability/MisleadingIndentationCheck.cpp
72 ↗(On Diff #88182)

Will it be useful to add a note pointing to the control statement and saying "did you mean this line to be inside this if/while/for/..."?

79 ↗(On Diff #88182)

has(anyOf(ifStmt(), forStmt(), whileStmt())) would read better.

This revision now requires changes to proceed.Feb 13 2017, 8:11 AM
clang-tidy/readability/MisleadingIndentationCheck.cpp
42 ↗(On Diff #88182)

I would rename Inside to Inner. That would make InnerLoc more "consistent".

48 ↗(On Diff #88182)

nit: write "const auto *CurrentWhile...". missing "*"?

test/clang-tidy/readability-misleading-indentation.cpp
21 ↗(On Diff #88182)

I am skeptic about this warning message.

Why does it say "potential". I would say that in this test case the indentation _is_ "dangling".

The message is not very clear to me. I personally don't intuitively understand what is wrong without looking at the code.

I don't know what it should say. Maybe:

different indentation for 'if' and 'else'
xazax.hun updated this revision to Diff 88330.Feb 14 2017, 1:07 AM
xazax.hun edited edge metadata.
xazax.hun marked 7 inline comments as done.
  • Added a note to make it easier to understand the diagnostics.
  • Reworded the error message about dangling else.
  • Fixed other review comments.

Thank you for the review!

clang-tidy/readability/MisleadingIndentationCheck.cpp
79 ↗(On Diff #88182)

I agree but unfortunately that does not compile.

test/clang-tidy/readability-misleading-indentation.cpp
21 ↗(On Diff #88182)

Thank you, good idea!

alexfh accepted this revision.Feb 14 2017, 2:07 AM

LG

clang-tidy/readability/MisleadingIndentationCheck.cpp
79 ↗(On Diff #88182)

Yep, with matchers you never know what will compile ;)

Maybe has(stmt(anyOf(ifStmt(), forStmt(), whileStmt())))? If this also doesn't work, then it's fine as is.

This revision is now accepted and ready to land.Feb 14 2017, 2:07 AM

Gábor, thank you for picking up this patch and finishing it!

This revision was automatically updated to reflect the committed changes.
xazax.hun added inline comments.Feb 14 2017, 2:15 AM
clang-tidy/readability/MisleadingIndentationCheck.cpp
79 ↗(On Diff #88182)

That did the trick, thanks :)