This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] readability-function-size: fix nesting level calculation
ClosedPublic

Authored by lebedev.ri on Jun 14 2017, 4:55 AM.

Details

Summary

A followup for D32942.

Malcolm Parsons has provided a valid testcase that the initial version of the check complained about nested if's.
As it turns out, the culprit is the partially un-intentional switch fallthrough.
So rewrite the NestingThreshold logic without ab-using+mis-using that switch with fallthrough, and add testcases with nested if' where there should be a warning and shouldn't be a warning. This results in a cleaner, simpler code, too.

I guess, now it would be actually possible to pick some reasonable default for NestingThreshold setting.

Fixes PR33454.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 14 2017, 4:55 AM
lebedev.ri planned changes to this revision.Jun 14 2017, 5:18 AM

Hmm, this is moving in the right direction, but now it invalidated (decreases the nesting level) too eagerly

wangxindsb removed a subscriber: wangxindsb.
lebedev.ri edited the summary of this revision. (Show Details)

Ok, i really messed up the initial D32942 :)
Malcolm, thank you for bothering to report :)
This should be so much better.

lebedev.ri edited the summary of this revision. (Show Details)Jun 14 2017, 6:22 AM
clang-tidy/readability/FunctionSizeCheck.cpp
38 ↗(On Diff #102533)

I don't think this assert adds anything.

62 ↗(On Diff #102533)

Comments are sentences, so start with a capital letter.

lebedev.ri marked an inline comment as done.

Fix spelling.

clang-tidy/readability/FunctionSizeCheck.cpp
38 ↗(On Diff #102533)

Yes, however the original if did have that check.
I think, having an assert here may help to not break this in the future.

Simplify it even further by moving the logic into it's proper place - TraverseCompoundStmt()

What do you expect for this?

if (true)
    if (true)
        if (true) {
            int j;
        }

What do you expect for this?

if (true)
    if (true)
        if (true) {
            int j;
        }

that it is equivalent to

if (true && true && true) { // 1
  int j;
}

This was the intent of that option. There is only one compound statement in your example. All the docs say that it counts compound statements
https://github.com/llvm-mirror/clang-tools-extra/blob/9fd3636de8d7034ca4640939fefebd9833ef9ea0/docs/clang-tidy/checks/readability-function-size.rst

.. option:: NestingThreshold

    Flag compound statements ...

My coding standards require the {}, so while I may not agree with your definition of nesting, it doesn't matter.

My coding standards require the {}, so while I may not agree with your definition of nesting, it doesn't matter.

Well, seeing readability-deep-nesting.cpp in the issue, i suppose the definition might be adjusted to fit that too, to avoid having more than one check doing just slightly different checking...

LGTM!

clang-tidy/readability/FunctionSizeCheck.cpp
58 ↗(On Diff #102548)

Missing full stop.

malcolm.parsons accepted this revision.Jun 14 2017, 9:37 AM
This revision is now accepted and ready to land.Jun 14 2017, 9:37 AM
This revision was automatically updated to reflect the committed changes.