[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Aaron Ballman via Phabricator via cfe-commits]

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