aaron.ballman added a comment.
I'm sorry for the delay in reviewing this; I'm not certain how it fell off my
radar for so long!
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:32
+ *
+ * Handle = either a pointer or reference
+ * Value = everything else (Type variable_name;)
----------------
Do you intend to support Obj-C object pointers as well?
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:141-142
+void ConstCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+ return;
+
----------------
Why is this check disabled for C code?
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:149
+
+ // Match local variables, that could be const.
+ // Example: `int i = 10`, `int i` (will be used if program is correct)
----------------
Drop the comma; that -> which
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:152-153
+ const auto LocalValDecl = varDecl(allOf(
+ isLocal(), hasInitializer(anything()), unless(ConstType),
+ unless(ConstReference), unless(TemplateType), unless(isImplicit())));
+
----------------
Can this use `unless(anyOf(...))` instead?
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:164-165
+void ConstCheck::check(const MatchFinder::MatchResult &Result) {
+ if (!getLangOpts().CPlusPlus)
+ return;
+
----------------
No need for this -- `check()` won't be called unless there are registered
matchers.
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:167
+
+ const auto *Scope = Result.Nodes.getNodeAs<CompoundStmt>("scope");
+ assert(Scope && "Did not match scope for local variable");
----------------
Can you pick a slightly different name -- `Scope` is the name of a type in the
`clang` namespace.
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:190
+ // TODO Implement automatic code transformation to add the const.
+ diag(Variable->getLocStart(), "variable %0 of type %1 can be declared const")
+ << Variable << Variable->getType();
----------------
const -> 'const'
================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.h:25
+
+/// This check warns for every variable, that could be declared as const, but
+/// isn't.
----------------
Grammar nit, perhaps: `This check warns on variables which could be declared
const but are not.`
================
Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:44
+ CheckFactories.registerCheck<ConstCheck>(
+ "cppcoreguidelines-const");
CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
----------------
This name leaves a bit to be desired. How about
`cppcoreguidelines-const-correctness`?
================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-const.rst:6
+
+This check implements detection of local variables that could be declared as
+``const``, but are not. Declaring variables as ``const`` is required by many
----------------
that -> which
================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-const.rst:8
+``const``, but are not. Declaring variables as ``const`` is required by many
+coding guidelines for example
+`CppCoreGuidelines ES.25
<https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on>`_
----------------
coding guidelines for example -> coding guidelines, such as:
================
Comment at: docs/clang-tidy/checks/cppcoreguidelines-const.rst:12
+
+Please note that this analysis is type based only. Variables that are not
modified
+but non-const handles might escape out of the scope are not diagnosed as
potential
----------------
type based -> type-based
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
