[clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.

[aaron.ballman]

clang-tidy/utils/TypeTraits.cpp

20

I would prefer this be used locally instead of at namespace scope to avoid potential name collisions.

32

!Record instead of explicit comparison.

34

Why use the matcher at all? You have the record decl, it can give you the copy constructor, and you can directly inspect whether that is deleted or not.

test/clang-tidy/performance-unnecessary-value-param.cpp

27

This is not a move-only type. Because you have a user-declared copy constructor (or destructor), the move constructor will not be implicitly declared. See [class.copy]p9 for details.

[alexfh]

clang-tidy/utils/TypeTraits.cpp

20

Note, that file-level using namespace ast_matchers is rather common in Clang tools, so I wouldn't bother here. That said, I'm fine either way.

[flx]

clang-tidy/utils/TypeTraits.cpp

32

Does this make a difference for code generation or is this just a convention here for conciseness?

[aaron.ballman]

clang-tidy/utils/TypeTraits.cpp

30

No need to pass in Context any longer.

32

Should be no codegen difference in practice; it's just conciseness.

34

Can just use const auto * for this, but I'm fine either way.

[alexfh]

clang-tidy/utils/TypeTraits.cpp

24

Now seems to be unused.

[etienneb]

clang-tidy/utils/TypeTraits.cpp

34

I prefer const auto * too

43

nit: const ASTContext &Context

[etienneb]

clang-tidy/utils/TypeTraits.cpp

23

Should this be lifted to 'type_traits' ?

The same function exists into clang-tidy/utils/TypeTraits.cpp

namespace {
bool classHasTrivialCopyAndDestroy(QualType Type) {
  auto *Record = Type->getAsCXXRecordDecl();
  return Record && Record->hasDefinition() &&
         !Record->hasNonTrivialCopyConstructor() &&
         !Record->hasNonTrivialDestructor();
}
} // namespace

Alex, any toughs?

30

ditto, to be lifted or not?

[flx]

clang-tidy/utils/TypeTraits.cpp

23

I'm not following. This is the same file.

30

What do you mean with lifted? Exposed in the header file?

43

isTriviallyCopyablType below takes non-const ASTContext.

[etienneb]

clang-tidy/utils/TypeTraits.cpp

23

My bad! I didn't saw the file change when reading. Forget about it.

30

ditto. I was thinking this was in the checker file.
ignore it.

[etienneb]

clang-tidy/utils/TypeTraits.cpp

43

You're right too.
But, it's possible to fix these prototypes:
http://reviews.llvm.org/D20226

[alexfh]

Looks good after addressing Etienne's comment.

Thanks!

[flx]

clang-tidy/utils/TypeTraits.cpp

43

Updated to const reference now that your patch is in.

clang-tidy/utils/TypeTraits.cpp

 //===--- TypeTraits.cpp - clang-tidy---------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "TypeTraits.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 namespace type_traits {
 
+using namespace ::clang::ast_matchers;
+
 namespace {
 bool classHasTrivialCopyAndDestroy(QualType Type) {
   auto *Record = Type->getAsCXXRecordDecl();
   return Record && Record->hasDefinition() &&
          !Record->hasNonTrivialCopyConstructor() &&
          !Record->hasNonTrivialDestructor();
 }
+
+bool hasDeletedCopyConstructor(QualType Type, ASTContext &Context) {
+  auto *Record = Type->getAsCXXRecordDecl();
+  if (Record == nullptr || !Record->hasDefinition())
+    return false;
+  auto Matches = match(cxxRecordDecl(hasMethod(
+                           cxxConstructorDecl(isCopyConstructor(), isDeleted())
+                               .bind("constructor"))),
+                       *Record, Context);
+  return !Matches.empty();
+}
+
 } // namespace
 
 llvm::Optional<bool> isExpensiveToCopy(QualType Type, ASTContext &Context) {
   if (Type->isDependentType())
     return llvm::None;
   return !Type.isTriviallyCopyableType(Context) &&
-         !classHasTrivialCopyAndDestroy(Type);
+         !classHasTrivialCopyAndDestroy(Type) &&
+         !hasDeletedCopyConstructor(Type, Context);
 }
 
 bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
                                            const ASTContext &Context) {
   const auto *ClassDecl = dyn_cast<CXXRecordDecl>(&RecordDecl);
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
     return true;

test/clang-tidy/performance-unnecessary-value-param.cpp

 class SomewhatTrivial {
  public:
   SomewhatTrivial();
   SomewhatTrivial(const SomewhatTrivial&) = default;
   ~SomewhatTrivial() = default;
   SomewhatTrivial& operator=(const SomewhatTrivial&);
 };
 
+struct MoveOnlyType {
+  MoveOnlyType(const MoveOnlyType&) = delete;
+  ~MoveOnlyType();
+  void constMethod() const;
+};
+
 void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
 // CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj);
 void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) {
   // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'Obj' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
   // CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj) {
 }
 
 void positiveExpensiveValue(ExpensiveToCopyType Obj);
struct NegativeOverriddenMethod : public VirtualMethod {
   }
 };
 
 struct NegativeDeletedMethod {
   ~NegativeDeletedMethod() {}
   NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
   // CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
 };
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+  M.constMethod();
+}