PiotrZSL added a comment.
Try to move some code to separate file, some utilty/...
Add tests with tyepdefs.
Add tests with class derive from type that it gets from template.
Probably this code will be used in few more checks in future.
try implement some virtual base validation,
add some cache so we could avoid validating same class/method again.
Maybe change this into separate class.
Anyway good work..
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:21
+
+enum CanMoveConstrcutorThrowResult {
+ CMCT_CanThrow, ///< We are sure the move constructor can throw
----------------
Constrcutor -> Constructor
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:27
+
+CanMoveConstrcutorThrowResult evaluateFunctionEST(const FunctionDecl *Func) {
+ const auto *FunProto = Func->getType()->getAs<FunctionProtoType>();
----------------
consider moving this all from this file to some utils/something, and don't
restrict this only to move operator/constructor but also support
destructor/other constructors and so on.
Simply if is not default, ten use evaluateFunctionEST, if is default, then try
analyse as you doing now.
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:31
+ return CMCT_Unknown;
+
+ switch (FunProto->canThrow()) {
----------------
i think that you may still need to use isUnresolvedExceptionSpec here, and
return Unknown if that is true
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:61
+ if (auto *FieldRecordType =
+ Context.getBaseElementType(FDecl->getType())->getAs<RecordType>()) {
+ if (const auto *FieldRecordDecl =
----------------
getType()->getUnqualifiedDesugaredType()->getAsCXXRecordDecl();
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:81
+ // We assume non record types cannot throw
+ return CMCT_CannotThrow;
+}
----------------
but we shoudn't, we should test now fields of that record.... because it may
not have move constructor... (in theory). Probably we should also check if it
has constructor, but is's implicit one, then we should also check fields and
bases of this class instead of trying to check constructor.
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:95
+ const CXXRecordDecl *RecordDecl = MethodDecl->getParent();
+ if (!RecordDecl || RecordDecl->isInvalidDecl())
+ return CMCT_Unknown;
----------------
isInvalidDecl looks redudant.
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:98-100
+ const ASTContext &Context = FuncDecl->getASTContext();
+ if (Context.getRecordType(RecordDecl).isNull())
+ return CMCT_Unknown;
----------------
this looks redudant..
================
Comment at:
clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:131
Finder->addMatcher(
cxxMethodDecl(anyOf(cxxConstructorDecl(),
hasOverloadedOperatorName("=")),
unless(isImplicit()), unless(isDeleted()))
----------------
use isMoveConstructor() here, instead of using Ctor->isMoveConstructor() later.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146922/new/
https://reviews.llvm.org/D146922
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
