[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.

Looks ok, would be good to run this on some code base.



================
Comment at: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:22
+      cxxMethodDecl(anyOf(cxxConstructorDecl(isMoveConstructor()),
+                          hasOverloadedOperatorName("=")),
                     unless(isImplicit()), unless(isDeleted()))
----------------
use isMoveAssignmentOperator instead of hasOverloadedOperatorName


================
Comment at: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:23
+                          hasOverloadedOperatorName("=")),
                     unless(isImplicit()), unless(isDeleted()))
           .bind("decl"),
----------------
move those 2 to begining...


================
Comment at: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:30
     const MatchFinder::MatchResult &Result) {
   if (const auto *Decl = Result.Nodes.getNodeAs<CXXMethodDecl>("decl")) {
     bool IsConstructor = false;
----------------
invert this if to reduce nesting.
```
const auto *Decl = Result.Nodes.getNodeAs<CXXMethodDecl>("decl")
if (!Decl)
    return;
```


================
Comment at: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:32-36
     if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Decl)) {
-      if (!Ctor->isMoveConstructor())
-        return;
       IsConstructor = true;
     } else if (!Decl->isMoveAssignmentOperator()) {
       return;
     }
----------------
IsConstructor = CXXConstructorDecl::classof(Decl);


================
Comment at: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:39
+    if (SpecAnalyzer.analyze(Decl) !=
+        utils::ExceptionSpecAnalyzer::State::Throwing) {
       return;
----------------
remove {}, leave just
```  
if (SpecAnalyzer.analyze(Decl) !=     utils::ExceptionSpecAnalyzer::State::Throwing) return;
```


================
Comment at: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp:45-49
+    const auto *ProtoType = Decl->getType()->castAs<FunctionProtoType>();
+    const Expr *NoexceptExpr = ProtoType->getNoexceptExpr();
+    if (NoexceptExpr) {
+      NoexceptExpr = NoexceptExpr->IgnoreImplicit();
+      if (!isa<CXXBoolLiteralExpr>(NoexceptExpr)) {
----------------
woudn't getExceptionSpecInfo() != EST_NoexceptFalse do a trick here ?


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:32
+  // Check if the function has already been analyzed and reuse that result.
+  if (FunctionCache.count(FuncDecl) == 0) {
+    State = analyzeImpl(FuncDecl);
----------------
use FunctionCache.find to avoid double search in line 32 and 38 (and reuse result)


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:36
+    // Cache the result of the analysis.
+    FunctionCache.insert(std::make_pair(FuncDecl, State));
+  } else
----------------
try_emplace ?


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:44
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeUnresolvedOrDefaulted(
+    const FunctionDecl *FuncDecl) {
----------------
pass FunctionProtoType also to this function


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:50-52
+  const auto *MethodDecl = cast<CXXMethodDecl>(FuncDecl);
+  if (!MethodDecl)
+    return State::Unknown;
----------------
technically you could change this class to operator on CXXMethodDecl since begining


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:76
+
+  // FIXME: There may still be a way to figure out if the field can throw or not
+  return State::Unknown;
----------------
check if this is trivial type, if its trivial type then cannot throw...
FDecl->getType()->isTrivialType(FDecl->getAstContext());


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:95
+                                     DefaultableMemberKind Kind,
+                                     SkipMethods SkipMethods) {
+  if (!RecordDecl)
----------------
Can we hit some endless recursion here ?
Maybe so protection against checking Record that we currently checking.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:108-114
+  BasesToVisit Bases = isConstructor(Kind)
+                           ? BasesToVisit::VisitPotentiallyConstructedBases
+                           : BasesToVisit::VisitAllBases;
+
+  if (Bases == BasesToVisit::VisitPotentiallyConstructedBases)
+    Bases = RecordDecl->isAbstract() ? BasesToVisit::VisitNonVirtualBases
+                                     : BasesToVisit::VisitAllBases;
----------------
I'm not sure if we need to be so picky...
In short we could check all bases.
Virtual, Abstract or not...


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.cpp:155
+ExceptionSpecAnalyzer::State
+ExceptionSpecAnalyzer::analyzeFunctionEST(const FunctionDecl *FuncDecl) {
+  const auto *FuncProto = FuncDecl->getType()->getAs<FunctionProtoType>();
----------------
pass FunctionProtoType also to this function, to avoid double checking


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h:23
+public:
+  enum class State : std::int8_t {
+    Throwing,    ///< This function has been declared as possibly throwing.
----------------
No need for std::int8_t, and if you really want then use std::uint8_t


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionSpecAnalyzer.h:78
+
+  DefaultableMemberKind getDefaultableMemberKind(const FunctionDecl *FuncDecl);
+
----------------
this also can be static


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146922/new/

https://reviews.llvm.org/D146922