Page MenuHomePhabricator

[clang-tidy] Add performance-prefer-preincrement check
AcceptedPublic

Authored by njames93 on Jan 10 2020, 8:00 PM.

Details

Summary

Adds a new clang-tidy check that detects postfix increments and decrements that can be swapped for their prefix counterparts. Tried to be as thorough as possible with detecting if the result of the operation is used by a parent. I ran this check over llvm and clang lib and the result would build OK.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-preincrement.rst
30 ↗(On Diff #237480)

Please highlight default value with single back-ticks.

It'll be reasonable to submit patches generated by check into trunk. May be after branching release.

njames93 updated this revision to Diff 237490.Jan 11 2020, 4:45 AM
  • Small refactor
njames93 marked an inline comment as done.Jan 11 2020, 4:49 AM

could this do with an alias into performance?

could this do with an alias into performance?

Sure. At least such tricky questions were asked on interviews decade ago :-)

I only have some naming nits

  • incriment -> increment
  • decriment -> decrement

Maybe rename the check llvm-prefer-preincrement -> llvm-prefer-pre-increment
and rename the files and classes PreferPreincrementCheck -> PreferPreIncrementCheck

clang-tools-extra/clang-tidy/llvm/PreferPreincrementCheck.cpp
38 ↗(On Diff #237490)

Does the checker give a lot of false positives?

njames93 updated this revision to Diff 237499.Jan 11 2020, 7:58 AM
njames93 marked 2 inline comments as done.
  • Added alias for performance module
  • renamed check to prefer-pre-increment
njames93 updated this revision to Diff 237500.Jan 11 2020, 8:01 AM
  • Renamed incriment in test cases to increment
njames93 added inline comments.Jan 11 2020, 8:03 AM
clang-tools-extra/clang-tidy/llvm/PreferPreincrementCheck.cpp
38 ↗(On Diff #237490)

First time I tried it there were lots of false positives. But after adding all those Stmt checks it appears to do a rather good job

njames93 updated this revision to Diff 237502.Jan 11 2020, 8:10 AM
  • 99% sure I have all incriments renamed now
Eugene.Zelenko added inline comments.Jan 11 2020, 8:11 AM
clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst
9

Please fix check name.

  • 99% sure I have all incriments renamed now

I checked and can't find them anymore, thanks.

njames93 updated this revision to Diff 237505.Jan 11 2020, 8:47 AM
  • Fix perf alias documentation
njames93 marked an inline comment as done.Jan 11 2020, 8:48 AM
njames93 updated this revision to Diff 237517.Jan 11 2020, 11:25 AM
  • Rebased onto trunk fixing merge issues

Unit tests: pass. 61775 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 238082.Jan 14 2020, 1:13 PM
  • fix spelling mistake

Happy to see this check!

clang-tools-extra/docs/ReleaseNotes.rst
199–202

Are we really really sure this is the correct relation direction?
This isn't an llvm-specific guideline that may be applicable to other code,
but a known generic C++ guideline that llvm coding guide follows.

Unit tests: pass. 61861 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 marked an inline comment as done.Jan 14 2020, 1:44 PM
njames93 added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
199–202

You're probably right, I added this to llvm first, then thought about alias. Which module should its primary be
I'd say performancepersonally. Cppcoreguidelines has 1 little note about it but I dont think that justifies putting the check in there.

lebedev.ri added inline comments.Jan 14 2020, 1:49 PM
clang-tools-extra/docs/ReleaseNotes.rst
199–202

I would agree with performance module.
Putting it in cppcoreguidelines would be the same - why there, it's not an Cppcoreguidelines invention?

njames93 updated this revision to Diff 238104.Jan 14 2020, 2:31 PM
  • moved check from llvm module to performance
njames93 retitled this revision from [clang-tidy] Add llvm-prefer-preincrement check to [clang-tidy] Add performance-prefer-preincrement check.Jan 14 2020, 2:32 PM
njames93 marked 2 inline comments as done.
  • moved check from llvm module to performance

Thank you!

Unit tests: pass. 61861 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 238740.Jan 17 2020, 4:12 AM
  • Streamlined replacement application

Unit tests: pass. 61943 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: fail. 61939 tests passed, 5 failed and 782 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

njames93 updated this revision to Diff 239988.Jan 23 2020, 12:50 PM
  • Rebase trunk and fix a few nits with test file

Unit tests: fail. 62129 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Gonna throw it out there, those pre merge fails aren't anything to do with this patch

Yeah, the libc++ failures are not your fault :)
My 2 cents added.

clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp
63 ↗(On Diff #239988)

Why would you deactivate the check for c-code?
I think the for(int i = 0; i < 10; ++i) could be a requirement in c-coding styles, too.

86 ↗(On Diff #239988)

i think pre and post instead of capitalized.

clang-tools-extra/test/clang-tidy/checkers/performance-prefer-pre-increment-disable-cpp-opcalls.cpp
11 ↗(On Diff #239988)

Please extract the common iterator class into a header and include it in the tests. this is done in other tests as well, just grep for include.

44 ↗(On Diff #239988)

Test-cases that I would like to see:

  • only the post-fix operator is overloaded for the class --> best-case this is detected and a fix is not provided
  • iterator-inheritance: the base-class provides the operator-overloads --> does matching work? There might be an implicit cast for example
  • the iterator-type is type-dependent --> maybe fixing should not be done or even the warning should not be emitted, because there might be only a post-fix available in some instantiations (see point 1). I do mean something like this template <typename T> void f() { T::iterator it; it++; }
njames93 updated this revision to Diff 240402.Jan 25 2020, 5:00 PM
njames93 marked 5 inline comments as done.
  • Address some nits, more test cases need adding though
clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp
63 ↗(On Diff #239988)

I only deactivate operator calls for c-code as they aren't a thing in c. The unary operation expr still runs in c-code

clang-tools-extra/test/clang-tidy/checkers/performance-prefer-pre-increment-disable-cpp-opcalls.cpp
44 ↗(On Diff #239988)

There are test cases for only post fix operator overloading. basically it doesn't warn or provided a fix it as that isn't valid. I feel like there could be a seperate check that detects classes that overload operator++(int) but not operator++() but thats not what this check is for.
I'll take a look at the other cases tomorrow

Unit tests: fail. 62197 tests passed, 1 failed and 815 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_class/lock.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

njames93 updated this revision to Diff 240584.Jan 27 2020, 7:43 AM
njames93 marked an inline comment as done.
  • Added test case when the inc and dec operators are found in a base class
clang-tools-extra/test/clang-tidy/checkers/performance-prefer-pre-increment-disable-cpp-opcalls.cpp
44 ↗(On Diff #239988)

Currently the base class provided operator overloads match normally.

For template dependent code it gets a little hazy. Basically if the type isn't known the operator will always appear as a UnaryOperator, maybe its safest to proceed by disabling fixes if the type isn't known, and maybe add an option to override that behaviour, WDYT?

Unit tests: pass. 62198 tests passed, 0 failed and 815 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

njames93 updated this revision to Diff 244555.Feb 13 2020, 6:00 PM
  • Added option to enable transforming template dependent types
njames93 updated this revision to Diff 244557.Feb 13 2020, 6:04 PM
  • Fix order in release notes

Running this over llvm with only clang and clang-tools-extra enabled this was the output. ninja check-all ran successfully with no errors. Didn't try transform TransformCxxOpCalls or TransformDependentType as they can be more dangerous

JonasToth accepted this revision.Sep 13 2020, 3:48 AM

Sorry for missing this review, i simply hadn't time :/
All my concerns are addressed and I think this is LGTM after rebasing.

This revision is now accepted and ready to land.Sep 13 2020, 3:48 AM
njames93 updated this revision to Diff 299443.Tue, Oct 20, 12:40 PM

Rebased and fix issues relating to rebase