njames93 added a subscriber: sammccall. njames93 added a comment. In D54943#3296191 <https://reviews.llvm.org/D54943#3296191>, @JonasToth wrote:
> In D54943#3292552 <https://reviews.llvm.org/D54943#3292552>, @0x8000-0000 > wrote: > >> @aaron.ballman - can this land for Clang14, or does it have wait for 15? >> >> Are there any other reviewers that can approve this? > > Sadly, cherry-picking to 14 is very unlikely, as this is not a bugfix. :/ > On the other hand, it missed so many releases so lets stay positive :) > > Aaron did the review and I think he should accept too. This won't get backported now, besides giving it a few months to test(and potentially iron out) on the dev branch isn't such a bad thing. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt:33 LINK_LIBS + clangAnalysis clangTidy ---------------- Will this work under clangd as I don't think that links in the clangAnalysis libraries @sammccall? ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:27 +AST_MATCHER(VarDecl, isLocal) { return Node.isLocalVarDecl(); } +AST_MATCHER_P(DeclStmt, containsDeclaration2, + ast_matchers::internal::Matcher<Decl>, InnerMatcher) { ---------------- nit ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:84 + compoundStmt( + findAll(declStmt(allOf(containsDeclaration2( + LocalValDecl.bind("local-value")), ---------------- `allOf` is unnecessary here as `declStmt` is implicitly an `allOf` matcher. Also `findAll` isn't the right matcher in this case, you likely want `forEachDescendant`. `findAll` will try and match on the `CompoundStmt` which is always going to fail. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:102-105 + // There have been crashes on 'Variable == nullptr', even though the matcher + // is not conditional. This comes probably from 'findAll'-matching. + if (!Variable || !LocalScope) + return; ---------------- This sounds like a bug that should be raised with ASTMatchers, if its reproducible. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:160 + using namespace utils::fixit; + using llvm::Optional; + if (VC == VariableCategory::Value && TransformValues) { ---------------- This is unnecessary, `llvm::Optional` already has an alias inside the clang namespace. ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:162-168 + if (Optional<FixItHint> Fix = addQualifierToVarDecl( + *Variable, *Result.Context, DeclSpec::TQ_const, + QualifierTarget::Value, QualifierPolicy::Right)) { + Diag << *Fix; + // FIXME: Add '{}' for default initialization if no user-defined default + // constructor exists and there is no initializer. + } ---------------- DiagnosticBuilders accept optional fixits as syntactic sugar. Same goes for below. ================ Comment at: clang-tools-extra/docs/ReleaseNotes.rst:108-112 +- New :doc:`cppcoreguidelines-const-correctness + <clang-tidy/checks/cppcoreguidelines-const-correctness>` check. + + Detects unmodified local variables and suggest adding ``const`` if the transformation is possible. + ---------------- This'll need rebasing after branching. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp:3-6 +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformReferences', value: 1}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \ ---------------- ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp:3-5 +// RUN: [{key: "cppcoreguidelines-const-correctness.AnalyzeValues", value: 1},\ +// RUN: {key: "cppcoreguidelines-const-correctness.WarnPointersAsValues", value: 1}, \ +// RUN: {key: "cppcoreguidelines-const-correctness.TransformPointersAsValues", value: 1},\ ---------------- ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:3-5 +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1},\ +// RUN: {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \ ---------------- ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp:3-5 +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \ ---------------- ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:3-5 +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \ +// RUN: {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \ ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54943/new/ https://reviews.llvm.org/D54943 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
