[clang-tidy] misc-unconventional-assign-operator suggest to use rvalue references in C++03 mode

[tetsuo-cpp]

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=27702
I wasn't sure how this type of thing is usually tested. So any advice would be appreciated.
check-llvm, check-clang and check-clang-tools are clean for me.
C++98

tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ cat compile_commands.json
[
{
  "directory": "/home/tetsuo/dev/llvm-project/test",
  "command": "/usr/bin/c++      -std=gnu++98 -o CMakeFiles/test.dir/test.cpp.o -c /home/tetsuo/dev/llvm-project/test/test.cpp",
  "file": "/home/tetsuo/dev/llvm-project/test/test.cpp"
}
]
tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ ../build/bin/clang-tidy --checks=misc-unconventional-assign-operator test.cpp
3053 warnings generated.
/home/tetsuo/dev/llvm-project/test/test.cpp:7:3: warning: operator=() should take 'Foo const&' or 'Foo' [misc-unconventional-assign-operator]
  Foo &operator=(Foo &Other) {
  ^
Suppressed 3052 warnings (3052 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

C++17

tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ cat compile_commands.json
[
{
  "directory": "/home/tetsuo/dev/llvm-project/test",
  "command": "/usr/bin/c++      -std=gnu++17 -o CMakeFiles/test.dir/test.cpp.o -c /home/tetsuo/dev/llvm-project/test/test.cpp",
  "file": "/home/tetsuo/dev/llvm-project/test/test.cpp"
}
]
tetsuo@garland-c-16-sgp1-01:~/dev/llvm-project/test$ ../build/bin/clang-tidy --checks=misc-unconventional-assign-operator test.cpp
5377 warnings generated.
/home/tetsuo/dev/llvm-project/test/test.cpp:7:3: warning: operator=() should take 'Foo const&', 'Foo&&' or 'Foo' [misc-unconventional-assign-operator]
  Foo &operator=(Foo &Other) {
  ^
Suppressed 5376 warnings (5376 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

[njames93]

Please add a test case for this

clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

76–77

Remove these lines and change below to be getLangOpts().CPlusPlus11. getLangOpts() is a protected method in ClangTidyCheck

[MaskRay]

In D75901#1914254, @njames93 wrote:

Please add a test case for this

More concretely, a check_clang_tidy -std=c++98 test in test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp

[njames93]

If you need help for the test case. Create a file in the clang-tools-extra/test/clang-tidy/checkers.
Name it something like misc-unconventional-assign-operator-pre11.cpp and paste this in

// RUN: %check_clang_tidy -std=c++98,c++03 %s misc-unconventional-assign-operator %t

struct BadArgument {
  BadArgument& operator=(BadArgument&);
  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&' or 'BadArgument'
};

[tetsuo-cpp]

Address review comments.

[tetsuo-cpp]

@njames93 @MaskRay
Thanks for helping me with testing. I'll remember this for next time.

I also saw this pattern of retrieving LangOptions from an ASTContext in some other checks (RedundantExpressionCheck and StaticAssertCheck) but I didn't change them to use getLangOpts since the ASTContext is used for other stuff.

[njames93]

LGTM
For the record check-clang-tools is sufficient for testing all clang tidy checks

[njames93]

In D75901#1916119, @tetsuo-cpp wrote:

@njames93 @MaskRay
Thanks for helping me with testing. I'll remember this for next time.

I also saw this pattern of retrieving LangOptions from an ASTContext in some other checks (RedundantExpressionCheck and StaticAssertCheck) but I didn't change them to use getLangOpts since the ASTContext is used for other stuff.

Its also not a good idea to change unrelated code in a review

[tetsuo-cpp]

In D75901#1920397, @njames93 wrote:

In D75901#1916119, @tetsuo-cpp wrote:

@njames93 @MaskRay
Thanks for helping me with testing. I'll remember this for next time.

I also saw this pattern of retrieving LangOptions from an ASTContext in some other checks (RedundantExpressionCheck and StaticAssertCheck) but I didn't change them to use getLangOpts since the ASTContext is used for other stuff.

Its also not a good idea to change unrelated code in a review

Understood. Thanks @njames93.
Could you please help me commit this change?

clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp

Finder->addMatcher(returnStmt(IsBadReturnStatement, forFunction(IsGoodAssign))
                      this);
 }
 
 void UnconventionalAssignOperatorCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (const auto *RetStmt = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt")) {
     diag(RetStmt->getBeginLoc(), "operator=() should always return '*this'");
   } else {
     static const char *const Messages[][2] = {
         {"ReturnType", "operator=() should return '%0&'"},
-        {"ArgumentType", "operator=() should take '%0 const&', '%0&&' or '%0'"},
+        {"ArgumentType",
+         getLangOpts().CPlusPlus11
+             ? "operator=() should take '%0 const&', '%0&&' or '%0'"
+             : "operator=() should take '%0 const&' or '%0'"},
         {"cv", "operator=() should not be marked '%1'"}};
 
     const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
     for (const auto &Message : Messages) {
       if (Result.Nodes.getNodeAs<Decl>(Message[0]))
         diag(Method->getBeginLoc(), Message[1])
             << Method->getParent()->getName()
             << (Method->isConst() ? "const" : "virtual");
     }
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator-precxx11.cpp

+// RUN: %check_clang_tidy -std=c++98,c++03 %s misc-unconventional-assign-operator %t
+
+struct BadArgument {
+  BadArgument &operator=(BadArgument &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&' or 'BadArgument'
+};