Page MenuHomePhabricator

[clang-tidy] Add performance-prefer-preincrement check
Needs ReviewPublic

Authored by njames93 on Fri, Jan 10, 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

njames93 created this revision.Fri, Jan 10, 8:00 PM
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.Sat, Jan 11, 4:45 AM
  • Small refactor
njames93 marked an inline comment as done.Sat, Jan 11, 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.Sat, Jan 11, 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.Sat, Jan 11, 8:01 AM
  • Renamed incriment in test cases to increment
njames93 added inline comments.Sat, Jan 11, 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.Sat, Jan 11, 8:10 AM
  • 99% sure I have all incriments renamed now
Eugene.Zelenko added inline comments.Sat, Jan 11, 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.Sat, Jan 11, 8:47 AM
  • Fix perf alias documentation
njames93 marked an inline comment as done.Sat, Jan 11, 8:48 AM
njames93 updated this revision to Diff 237517.Sat, Jan 11, 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.Tue, Jan 14, 1:13 PM
  • fix spelling mistake

Happy to see this check!

clang-tools-extra/docs/ReleaseNotes.rst
87–90

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.Tue, Jan 14, 1:44 PM
njames93 added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
87–90

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.Tue, Jan 14, 1:49 PM
clang-tools-extra/docs/ReleaseNotes.rst
87–90

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.Tue, Jan 14, 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.Tue, Jan 14, 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.Fri, Jan 17, 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