[PATCH] D146922: [clang-tidy] Fix false positve for defaulted move constructor in performance-noexcept-move-constructor

[Piotr Zegar via Phabricator via cfe-commits]

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