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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits