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

[aaron.ballman]

clang-tidy/utils/TypeTraits.cpp

20 ↗

(On Diff #56913)

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

32 ↗

(On Diff #56913)

!Record instead of explicit comparison.

34 ↗

(On Diff #56913)

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 ↗

(On Diff #56913)

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 ↗

(On Diff #56913)

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 ↗

(On Diff #56913)

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

[aaron.ballman]

clang-tidy/utils/TypeTraits.cpp

31 ↗

(On Diff #57035)

No need to pass in Context any longer.

33 ↗

(On Diff #57035)

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

35 ↗

(On Diff #57035)

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

[alexfh]

clang-tidy/utils/TypeTraits.cpp

22 ↗

(On Diff #57035)

Now seems to be unused.

[etienneb]

clang-tidy/utils/TypeTraits.cpp

35 ↗

(On Diff #57035)

I prefer const auto * too

44 ↗

(On Diff #57035)

nit: const ASTContext &Context

[etienneb]

clang-tidy/utils/TypeTraits.cpp

24 ↗

(On Diff #57035)

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?

31 ↗

(On Diff #57035)

ditto, to be lifted or not?

[flx]

clang-tidy/utils/TypeTraits.cpp

24 ↗

(On Diff #57035)

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

31 ↗

(On Diff #57035)

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

44 ↗

(On Diff #57035)

isTriviallyCopyablType below takes non-const ASTContext.

[etienneb]

clang-tidy/utils/TypeTraits.cpp

22 ↗

(On Diff #57129)

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

29 ↗

(On Diff #57129)

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

[etienneb]

clang-tidy/utils/TypeTraits.cpp

42 ↗

(On Diff #57129)

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

42 ↗

(On Diff #57129)

Updated to const reference now that your patch is in.

clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h

 #include "clang/AST/Type.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 namespace type_traits {
 
 // \brief Returns true If \c Type is expensive to copy.
-llvm::Optional<bool> isExpensiveToCopy(QualType Type, ASTContext &Context);
+llvm::Optional<bool> isExpensiveToCopy(QualType Type,
+                                       const ASTContext &Context);
 
 // \brief Returns true If \c Type is trivially default constructible.
 bool isTriviallyDefaultConstructible(QualType Type, const ASTContext &Context);
 
 // \brief Returns true If \c RecordDecl is trivially default constructible.
 bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
                                            const ASTContext &Context);
 
 } // type_traits
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_TYPETRAITS_H

clang-tools-extra/trunk/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 {
 
 namespace {
+
 bool classHasTrivialCopyAndDestroy(QualType Type) {
   auto *Record = Type->getAsCXXRecordDecl();
   return Record && Record->hasDefinition() &&
          !Record->hasNonTrivialCopyConstructor() &&
          !Record->hasNonTrivialDestructor();
 }
+
+bool hasDeletedCopyConstructor(QualType Type) {
+  auto *Record = Type->getAsCXXRecordDecl();
+  if (!Record || !Record->hasDefinition())
+    return false;
+  for (const auto *Constructor : Record->ctors()) {
+    if (Constructor->isCopyConstructor() && Constructor->isDeleted())
+      return true;
+  }
+  return false;
+}
+
 } // namespace
 
-llvm::Optional<bool> isExpensiveToCopy(QualType Type, ASTContext &Context) {
+llvm::Optional<bool> isExpensiveToCopy(QualType Type,
+                                       const ASTContext &Context) {
   if (Type->isDependentType())
     return llvm::None;
   return !Type.isTriviallyCopyableType(Context) &&
-         !classHasTrivialCopyAndDestroy(Type);
+         !classHasTrivialCopyAndDestroy(Type) &&
+         !hasDeletedCopyConstructor(Type);
 }
 
 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;

clang-tools-extra/trunk/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(MoveOnlyType &&) = default;
+  ~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();
+}