[clang-tidy]bugprone-unused-return-value ignore ++ and -- operator overloading

[HerrCai0907]

Fixes: #84705
Further fix for #84489

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Congcong Cai (HerrCai0907)

Changes

Fixes: #84705

---

Full diff: https://github.com/llvm/llvm-project/pull/84922.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp (+14-14)
• (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-avoid-assignment.cpp (+8)

diff --git a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
index 243fe47c2036b6..83b332fba1e2da 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
@@ -165,20 +165,20 @@ void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
 
 void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
   auto MatchedDirectCallExpr = expr(
-      callExpr(
-          callee(functionDecl(
-              // Don't match void overloads of checked functions.
-              unless(returns(voidType())),
-              // Don't match copy or move assignment operator.
-              unless(cxxMethodDecl(isOperatorOverloading(
-                  {OO_Equal, OO_PlusEqual, OO_MinusEqual, OO_StarEqual,
-                   OO_SlashEqual, OO_PercentEqual, OO_CaretEqual, OO_AmpEqual,
-                   OO_PipeEqual, OO_LessLessEqual, OO_GreaterGreaterEqual}))),
-              anyOf(
-                  isInstantiatedFrom(
-                      matchers::matchesAnyListedName(CheckedFunctions)),
-                  returns(hasCanonicalType(hasDeclaration(namedDecl(
-                      matchers::matchesAnyListedName(CheckedReturnTypes)))))))))
+      callExpr(callee(functionDecl(
+                   // Don't match void overloads of checked functions.
+                   unless(returns(voidType())),
+                   // Don't match copy or move assignment operator.
+                   unless(cxxMethodDecl(isOperatorOverloading(
+                       {OO_Equal, OO_PlusEqual, OO_MinusEqual, OO_StarEqual,
+                        OO_SlashEqual, OO_PercentEqual, OO_CaretEqual,
+                        OO_AmpEqual, OO_PipeEqual, OO_LessLessEqual,
+                        OO_GreaterGreaterEqual, OO_PlusPlus, OO_MinusMinus}))),
+                   anyOf(isInstantiatedFrom(
+                             matchers::matchesAnyListedName(CheckedFunctions)),
+                         returns(hasCanonicalType(hasDeclaration(
+                             namedDecl(matchers::matchesAnyListedName(
+                                 CheckedReturnTypes)))))))))
           .bind("match"));
 
   auto CheckCastToVoid =
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-avoid-assignment.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-avoid-assignment.cpp
index b4a41004adf894..5809da9ec27b38 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-avoid-assignment.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-avoid-assignment.cpp
@@ -10,6 +10,10 @@ struct S {
   S &operator=(S const &);
   S &operator=(S &&);
   S &operator+=(S);
+  S &operator++();
+  S &operator++(int);
+  S &operator--();
+  S &operator--(int);
 };
 
 S returnValue();
@@ -27,4 +31,8 @@ void bar() {
   a.operator=(returnRef());
 
   a += returnRef();
+  a++;
+  ++a;
+  a--;
+  --a;
 }

[EugeneZelenko]

Please mention changes in Release Notes.

[HerrCai0907]

Please mention changes in Release Notes.

release notes already includes this change.

[PiotrZSL]

Overall change is fine.
There is only one usage of isOperatorOverloading, so you could consider changing this into:
isOverloadAssigmentOperator and put all those operator kinds in a matcher instead of messing with SmallVector.

[PiotrZSL]

Just curios, what about operators outside class, like:

struct S {
  S(){};
  S(S const &);
  S(S &&);
};

  S &operator+=(S& s, S s2);

void test()
{
    S s;
    S s2;

    s += s2;

    operator+=(s, s2);
}

in AST those are functions, not methods:

-FunctionDecl <line:7:3, col:27> col:6 used operator+= 'S &(S &, S)'
| |-ParmVarDecl <col:17, col:20> col:20 s 'S &'
| `-ParmVarDecl <col:23, col:25> col:25 s2 'S':'S'

[PiotrZSL]

Few more tests would be also nice, with free standing operators.

[PiotrZSL]

getOverloadedOperator belongs to FunctionDecl, change this from Method to Function, and same in calee, in such case you will also cover calls to free standing operators called directly.

[PiotrZSL]

try changing this code in a way so callee woudn't be checked if cxxOperatorCallExpr were already checked.
you could do it like this:

anyOf(cxxOperatorCallExpr(unless(isAssignmentOverloadedOperatorCall()),
           unless(callee(functionDecl(isAssignmentOverloadedOperatorFunction()))))

You may also consider moving this before all those matchers::matchesAnyListedName, so names woudn't be checked for operators at all.

[5chmidti]

Should this check really ignore postfix increment/decrement? Or maybe this check ignores it and a new check diagnoses it++ vs ++it (where possible). What are your thoughts on this?

[HerrCai0907]

Should this check really ignore postfix increment/decrement? Or maybe this check ignores it and a new check diagnoses it++ vs ++it (where possible). What are your thoughts on this?

@5chmidti What does a new check diagnoses it++vs++it (where possible) mean?

[5chmidti]

@5chmidti What does a new check diagnoses it++vs++it (where possible) mean?

I mean this pattern:

void foo(std::string str) {
  auto iter = str.begin();
  iter++; // prefer ++iter;
  ++iter;

  sink(iter++);
  sink(++iter);

  for (auto i = str.begin(); i != str.end(); ++i) {}
  for (auto i = str.begin(); i != str.end(); i++) {} // prefer ++i
}

Where people write i++ but they really mean ++i. My question is if this check should ignore both of these cases, or if another check should detect this.

[HerrCai0907]

It should be another check in maybe performance, since this check wants to detect unused return instead of iterator.

[5chmidti]

LGTM from my side, please wait for @PiotrZSL to reconfirm approval/finish review