PiotrZSL added a comment. Overall good, my main concern is base class name & location.
================ Comment at: clang-tools-extra/clang-tidy/performance/NoexceptDestructorCheck.h:23 /// https://clang.llvm.org/extra/clang-tidy/checks/performance/noexcept-destructor.html -class NoexceptDestructorCheck : public ClangTidyCheck { +class NoexceptDestructorCheck : public utils::NoexceptFunctionCheck { public: ---------------- Consider putting "Base" into a name, so that it woudn't be considered a full check, and it can be in this module, no need to put it into utils ================ Comment at: clang-tools-extra/clang-tidy/performance/NoexceptDestructorCheck.h:25-26 public: NoexceptDestructorCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + : NoexceptFunctionCheck(Name, Context) {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; ---------------- we got c++17, so delegate constructor. using utils::NoexceptFunctionCheck::NoexceptFunctionCheck; ================ Comment at: clang-tools-extra/clang-tidy/performance/NoexceptDestructorCheck.h:28-30 std::optional<TraversalKind> getCheckTraversalKind() const override { return TK_IgnoreUnlessSpelledInSource; } ---------------- consider moving this into base class ================ Comment at: clang-tools-extra/clang-tidy/utils/NoexceptFunctionCheck.h:34-38 + virtual DiagnosticBuilder + reportMissingNoexcept(const FunctionDecl *FuncDecl) = 0; + + virtual void reportNoexceptEvaluatedToFalse(const FunctionDecl *FuncDecl, + const Expr *NoexceptExpr) = 0; ---------------- you can move those into protected. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153198/new/ https://reviews.llvm.org/D153198 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
