Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL269581: [clang-tidy] TypeTraits - Type is not expensive to
copy when it has a deleted… (authored by flx).
Changed prior to commit:
http://reviews.llvm.org/D20170?vs=57286=57287#toc
Repository:
rL LLVM
http://reviews.llvm.org/D20170
Files:
clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,42 @@
#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 isExpensiveToCopy(QualType Type, ASTContext ) {
+llvm::Optional isExpensiveToCopy(QualType Type,
+ const ASTContext ) {
if (Type->isDependentType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
- !classHasTrivialCopyAndDestroy(Type);
+ !classHasTrivialCopyAndDestroy(Type) &&
+ !hasDeletedCopyConstructor(Type);
}
bool recordIsTriviallyDefaultConstructible(const RecordDecl ,
Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h
===
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h
@@ -19,7 +19,8 @@
namespace type_traits {
// \brief Returns true If \c Type is expensive to copy.
-llvm::Optional isExpensiveToCopy(QualType Type, ASTContext );
+llvm::Optional isExpensiveToCopy(QualType Type,
+ const ASTContext );
// \brief Returns true If \c Type is trivially default constructible.
bool isTriviallyDefaultConstructible(QualType Type, const ASTContext );
Index:
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
===
---
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
+++
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,13 @@
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) {
@@ -169,3 +176,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) =
delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,42 @@
#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
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
flx added inline comments.
Comment at: clang-tidy/utils/TypeTraits.cpp:42
@@ -27,3 +41,3 @@
llvm::Optional isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
etienneb wrote:
> You're right too.
> But, it's possible to fix these prototypes:
> http://reviews.llvm.org/D20226
Updated to const reference now that your patch is in.
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
flx updated this revision to Diff 57286.
flx marked 3 inline comments as done.
http://reviews.llvm.org/D20170
Files:
clang-tidy/utils/TypeTraits.cpp
clang-tidy/utils/TypeTraits.h
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,13 @@
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) {
@@ -169,3 +176,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) =
delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.h
===
--- clang-tidy/utils/TypeTraits.h
+++ clang-tidy/utils/TypeTraits.h
@@ -19,7 +19,8 @@
namespace type_traits {
// \brief Returns true If \c Type is expensive to copy.
-llvm::Optional isExpensiveToCopy(QualType Type, ASTContext );
+llvm::Optional isExpensiveToCopy(QualType Type,
+ const ASTContext );
// \brief Returns true If \c Type is trivially default constructible.
bool isTriviallyDefaultConstructible(QualType Type, const ASTContext );
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,42 @@
#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 isExpensiveToCopy(QualType Type, ASTContext ) {
+llvm::Optional isExpensiveToCopy(QualType Type,
+ const ASTContext ) {
if (Type->isDependentType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
- !classHasTrivialCopyAndDestroy(Type);
+ !classHasTrivialCopyAndDestroy(Type) &&
+ !hasDeletedCopyConstructor(Type);
}
bool recordIsTriviallyDefaultConstructible(const RecordDecl ,
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,13 @@
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) {
@@ -169,3 +176,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.h
===
--- clang-tidy/utils/TypeTraits.h
+++ clang-tidy/utils/TypeTraits.h
@@ -19,7 +19,8 @@
namespace type_traits {
// \brief Returns true If \c Type is expensive to copy.
-llvm::Optional isExpensiveToCopy(QualType Type, ASTContext );
+llvm::Optional isExpensiveToCopy(QualType Type,
+ const ASTContext );
// \brief Returns true If \c Type is trivially default constructible.
bool isTriviallyDefaultConstructible(QualType Type, const ASTContext );
Index: clang-tidy/utils/TypeTraits.cpp
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Looks good after addressing Etienne's comment. Thanks! http://reviews.llvm.org/D20170 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
etienneb added inline comments.
Comment at: clang-tidy/utils/TypeTraits.cpp:42
@@ -27,3 +41,3 @@
llvm::Optional isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
You're right too.
But, it's possible to fix these prototypes:
http://reviews.llvm.org/D20226
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
etienneb added inline comments.
Comment at: clang-tidy/utils/TypeTraits.cpp:22
@@ -20,2 +21,3 @@
+
bool classHasTrivialCopyAndDestroy(QualType Type) {
auto *Record = Type->getAsCXXRecordDecl();
My bad! I didn't saw the file change when reading. Forget about it.
Comment at: clang-tidy/utils/TypeTraits.cpp:29
@@ +28,3 @@
+
+bool hasDeletedCopyConstructor(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
ditto. I was thinking this was in the checker file.
ignore it.
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
flx added inline comments.
Comment at: clang-tidy/utils/TypeTraits.cpp:24
@@ -20,2 +23,3 @@
+
bool classHasTrivialCopyAndDestroy(QualType Type) {
auto *Record = Type->getAsCXXRecordDecl();
etienneb wrote:
> 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?
I'm not following. This is the same file.
Comment at: clang-tidy/utils/TypeTraits.cpp:31
@@ +30,3 @@
+
+bool hasDeletedCopyConstructor(QualType Type, ASTContext ) {
+ auto *Record = Type->getAsCXXRecordDecl();
etienneb wrote:
> aaron.ballman wrote:
> > No need to pass in `Context` any longer.
> ditto, to be lifted or not?
What do you mean with lifted? Exposed in the header file?
Comment at: clang-tidy/utils/TypeTraits.cpp:44
@@ -27,3 +43,3 @@
llvm::Optional isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
etienneb wrote:
> nit: const ASTContext
isTriviallyCopyablType below takes non-const ASTContext.
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
flx updated this revision to Diff 57129.
flx marked 5 inline comments as done.
http://reviews.llvm.org/D20170
Files:
clang-tidy/utils/TypeTraits.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,13 @@
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) {
@@ -169,3 +176,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) =
delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,41 @@
#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 isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
- !classHasTrivialCopyAndDestroy(Type);
+ !classHasTrivialCopyAndDestroy(Type) &&
+ !hasDeletedCopyConstructor(Type);
}
bool recordIsTriviallyDefaultConstructible(const RecordDecl ,
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,13 @@
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) {
@@ -169,3 +176,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,41 @@
#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 isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
- !classHasTrivialCopyAndDestroy(Type);
+ !classHasTrivialCopyAndDestroy(Type) &&
+ !hasDeletedCopyConstructor(Type);
}
bool
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
etienneb added inline comments.
Comment at: clang-tidy/utils/TypeTraits.cpp:24
@@ -20,2 +23,3 @@
+
bool classHasTrivialCopyAndDestroy(QualType Type) {
auto *Record = Type->getAsCXXRecordDecl();
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?
Comment at: clang-tidy/utils/TypeTraits.cpp:31
@@ +30,3 @@
+
+bool hasDeletedCopyConstructor(QualType Type, ASTContext ) {
+ auto *Record = Type->getAsCXXRecordDecl();
aaron.ballman wrote:
> No need to pass in `Context` any longer.
ditto, to be lifted or not?
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
etienneb added a subscriber: etienneb.
Comment at: clang-tidy/utils/TypeTraits.cpp:35
@@ +34,3 @@
+return false;
+ for (const CXXConstructorDecl *Constructor : Record->ctors()) {
+if (Constructor->isCopyConstructor() && Constructor->isDeleted())
aaron.ballman wrote:
> Can just use `const auto *` for this, but I'm fine either way.
I prefer const auto * too
Comment at: clang-tidy/utils/TypeTraits.cpp:44
@@ -27,3 +43,3 @@
llvm::Optional isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
nit: const ASTContext
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
alexfh added inline comments. Comment at: clang-tidy/utils/TypeTraits.cpp:22 @@ +21,3 @@ + +using namespace ::clang::ast_matchers; + Now seems to be unused. http://reviews.llvm.org/D20170 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/TypeTraits.cpp:31
@@ +30,3 @@
+
+bool hasDeletedCopyConstructor(QualType Type, ASTContext ) {
+ auto *Record = Type->getAsCXXRecordDecl();
No need to pass in `Context` any longer.
Comment at: clang-tidy/utils/TypeTraits.cpp:33
@@ +32,3 @@
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (!Record || !Record->hasDefinition())
+return false;
Should be no codegen difference in practice; it's just conciseness.
Comment at: clang-tidy/utils/TypeTraits.cpp:35
@@ +34,3 @@
+return false;
+ for (const CXXConstructorDecl *Constructor : Record->ctors()) {
+if (Constructor->isCopyConstructor() && Constructor->isDeleted())
Can just use `const auto *` for this, but I'm fine either way.
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
flx added inline comments. Comment at: clang-tidy/utils/TypeTraits.cpp:32 @@ +31,3 @@ + auto *Record = Type->getAsCXXRecordDecl(); + if (Record == nullptr || !Record->hasDefinition()) +return false; aaron.ballman wrote: > `!Record` instead of explicit comparison. Does this make a difference for code generation or is this just a convention here for conciseness? http://reviews.llvm.org/D20170 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
flx removed rL LLVM as the repository for this revision.
flx updated this revision to Diff 57035.
flx marked 5 inline comments as done.
http://reviews.llvm.org/D20170
Files:
clang-tidy/utils/TypeTraits.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,13 @@
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) {
@@ -169,3 +176,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) =
delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,43 @@
#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 {
+
+using namespace ::clang::ast_matchers;
+
bool classHasTrivialCopyAndDestroy(QualType Type) {
auto *Record = Type->getAsCXXRecordDecl();
return Record && Record->hasDefinition() &&
!Record->hasNonTrivialCopyConstructor() &&
!Record->hasNonTrivialDestructor();
}
+
+bool hasDeletedCopyConstructor(QualType Type, ASTContext ) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (!Record || !Record->hasDefinition())
+return false;
+ for (const CXXConstructorDecl *Constructor : Record->ctors()) {
+if (Constructor->isCopyConstructor() && Constructor->isDeleted())
+ return true;
+ }
+ return false;
+}
+
} // namespace
llvm::Optional isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
- !classHasTrivialCopyAndDestroy(Type);
+ !classHasTrivialCopyAndDestroy(Type) &&
+ !hasDeletedCopyConstructor(Type, Context);
}
bool recordIsTriviallyDefaultConstructible(const RecordDecl ,
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,13 @@
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) {
@@ -169,3 +176,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,43 @@
#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 {
+
+using namespace ::clang::ast_matchers;
+
bool classHasTrivialCopyAndDestroy(QualType Type) {
auto *Record = Type->getAsCXXRecordDecl();
return Record && Record->hasDefinition() &&
!Record->hasNonTrivialCopyConstructor() &&
!Record->hasNonTrivialDestructor();
}
+
+bool hasDeletedCopyConstructor(QualType Type, ASTContext ) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (!Record || !Record->hasDefinition())
+return false;
+ for (const CXXConstructorDecl *Constructor : Record->ctors()) {
+if (Constructor->isCopyConstructor() && Constructor->isDeleted())
+ return true;
+ }
+ return false;
+}
+
} // namespace
llvm::Optional isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
return llvm::None;
return
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
alexfh added inline comments. Comment at: clang-tidy/utils/TypeTraits.cpp:20 @@ -18,1 +19,3 @@ +using namespace ::clang::ast_matchers; + aaron.ballman wrote: > I would prefer this be used locally instead of at namespace scope to avoid > potential name collisions. 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. Repository: rL LLVM http://reviews.llvm.org/D20170 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
aaron.ballman added a subscriber: aaron.ballman.
Comment at: clang-tidy/utils/TypeTraits.cpp:20
@@ -18,1 +19,3 @@
+using namespace ::clang::ast_matchers;
+
I would prefer this be used locally instead of at namespace scope to avoid
potential name collisions.
Comment at: clang-tidy/utils/TypeTraits.cpp:32
@@ +31,3 @@
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (Record == nullptr || !Record->hasDefinition())
+return false;
`!Record` instead of explicit comparison.
Comment at: clang-tidy/utils/TypeTraits.cpp:34
@@ +33,3 @@
+return false;
+ auto Matches = match(cxxRecordDecl(hasMethod(
+ cxxConstructorDecl(isCopyConstructor(), isDeleted())
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.
Comment at: test/clang-tidy/performance-unnecessary-value-param.cpp:27
@@ +26,3 @@
+struct MoveOnlyType {
+ MoveOnlyType(const MoveOnlyType&) = delete;
+ ~MoveOnlyType();
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.
Repository:
rL LLVM
http://reviews.llvm.org/D20170
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20170: [clang-tidy] TypeTraits - Type is not expensive to copy when it has a deleted copy constructor.
flx created this revision.
flx added reviewers: alexfh, sbenza.
flx added a subscriber: cfe-commits.
flx set the repository for this revision to rL LLVM.
Repository:
rL LLVM
http://reviews.llvm.org/D20170
Files:
clang-tidy/utils/TypeTraits.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,12 @@
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) {
@@ -169,3 +175,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) =
delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,42 @@
#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 ) {
+ 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 isExpensiveToCopy(QualType Type, ASTContext ) {
if (Type->isDependentType())
return llvm::None;
return !Type.isTriviallyCopyableType(Context) &&
- !classHasTrivialCopyAndDestroy(Type);
+ !classHasTrivialCopyAndDestroy(Type) &&
+ !hasDeletedCopyConstructor(Type, Context);
}
bool recordIsTriviallyDefaultConstructible(const RecordDecl ,
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -23,6 +23,12 @@
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) {
@@ -169,3 +175,7 @@
NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
// CHECK-FIXES: NegativeDeletedMethod& operator=(NegativeDeletedMethod N) = delete;
};
+
+void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
+ M.constMethod();
+}
Index: clang-tidy/utils/TypeTraits.cpp
===
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -10,26 +10,42 @@
#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 ) {
+ 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
