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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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



================
Comment at: clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp:63
+
+  if (!getLangOpts().CPlusPlus || !TransformCxxOpCalls)
+    return;
----------------
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. 


================
Comment at: clang-tools-extra/clang-tidy/performance/PreferPreIncrementCheck.cpp:86
+  DiagnosticBuilder Diag =
+      diag(Range.getBegin(), "Use Pre-%0 instead of Post-%0")
+      << (IsIncrement ? "increment" : "decrement");
----------------
i think `pre` and `post` instead of capitalized.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-prefer-pre-increment-disable-cpp-opcalls.cpp:11
+  Iterator(T *Pointer) : Current(Pointer) {}
+  T &operator*() const { return *Current; }
+  Iterator &operator++() { return ++Current, *this; }
----------------
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`.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-prefer-pre-increment-disable-cpp-opcalls.cpp:44
+};
+
+void foo() {
----------------
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++; }`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72553