[PATCH] D72553: [clang-tidy] Add performance-prefer-preincrement check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

In D72553#1815497 <https://reviews.llvm.org/D72553#1815497>, @njames93 wrote:

> could this do with an alias into performance?

If it was 1997, I'd say yes, but I am not aware of any optimizer that does so poorly with pre/post increment to suggest this is actually a performance-related check still today. Do we have any evidence that at O1 <https://reviews.llvm.org/owners/package/1/> or higher this has any impact whatsoever on performance? If not, then I'd say the alias shouldn't be there (because the check could be noisy on correct code bases).



================
Comment at: clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp:56
+    CheckFactories.registerCheck<PreferPreIncrementCheck>(
+        "performance-prefer-pre-increment");
     CheckFactories.registerCheck<TriviallyDestructibleCheck>(
----------------
I think the name is a bit unfortunate -- the check handles more than just incrementing. But there's not really a good neutral word like "crement" that covers both actions. "Adjustment" sort of comes close, but would be kind of awful for a check name. Alternative ideas are welcome here.


================
Comment at: clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp:21
+namespace {
+AST_MATCHER_P(UnaryOperator, isOperator, UnaryOperatorKind, Opcode) {
+  return Node.getOpcode() == Opcode;
----------------
Oh, neat, we have `unaryOperator(hasOperatorName("++"))` but that doesn't quite cut it here. :-D


================
Comment at: clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp:37-38
+void PreferPreIncrementCheck::registerMatchers(MatchFinder *Finder) {
+  // Ignore all unary ops with a parent decl or expr, those use the value
+  // returned. Reordering those would change the behaviour of the expression.
+  // FIXME: Add any more parents which could use the result of the operation.
----------------
Overloaded operators are a similar concern about changing the behavior of the expression. I think it's reasonably safe to assume that pre and post increment/decrement will typically do the same operations, but perhaps we should skip any overloaded operators that aren't idiomatic or aren't paired? e.g.,
```
struct S {
  S operator++(int);
}; // Only post-fix available, don't diagnose

struct T {
  int& operator++();
  int operator++(int);
}; // Not idiomatic return types, don't diagnose

struct U {
  S& operator++();
  T operator++(int);
}; // Oh god, this just hurts, don't diagnose
```


================
Comment at: clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp:42
+      unless(anyOf(hasParent(decl()), hasParent(expr()),
+                   hasParent(returnStmt()), hasParent(cxxThrowExpr())));
+  auto BoundExpr = expr().bind("ignore");
----------------
I think `hasParent(cxxThrowExpr())` is already covered by `hasParent(expr())`, right?

Also, this may not be quite right -- what about a case like this: `sizeof(i++)` where there's no performance reason to prefer one or the other?


================
Comment at: clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp:88
+                                           bool IsIncrement, bool WarnOnly) {
+  // Warn for all occurances, but don't fix macro usage.
+  DiagnosticBuilder Diag =
----------------
Typo: occurances -> occurrences


================
Comment at: clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp:90
+  DiagnosticBuilder Diag =
+      diag(Range.getBegin(), "Use pre-%0 instead of post-%0")
+      << (IsIncrement ? "increment" : "decrement");
----------------
clang-tidy diagnostics are not capitalized like that -- also, this diagnostic doesn't really say what's wrong with the user's code. How about: `pre-%0 may have better performance characteristics than post-%0` or something along those lines?


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:199-202
+- New alias :doc:`performance-prefer-pre-increment
+  <clang-tidy/checks/performance-prefer-pre-increment>` to
+  :doc:`llvm-prefer-pre-increment
+  <clang-tidy/checks/llvm-prefer-pre-increment>` was added.
----------------
lebedev.ri wrote:
> njames93 wrote:
> > lebedev.ri wrote:
> > > 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.
> > 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 [[ https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enforcement-8 | 1 little note ]] about it but I dont think that justifies putting the check in there.
> I would agree with `performance` module.
> Putting it in cppcoreguidelines would be the same - why there, it's not an Cppcoreguidelines invention?
I'm rather opposed to putting it in the `performance` module without evidence that this actually impacts performance. I find that this module tends to be really chatty already, and this check is likely to trigger *a lot* on real world code bases, so I want to make sure it's justified as a performance issue (looking around online suggests that this is not the case any longer in most situations and this has become more of a readability issue than a performance one).



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance-prefer-pre-increment.rst:7
+Flags usages of the unary postfix increment and decrement operators where the
+result isnt used which could be replaced with the more efficient prefix variety.
+
----------------
isnt used which -> isn't used and


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72553/new/

https://reviews.llvm.org/D72553