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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
