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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

The functionality is looking good, aside from a few small nits remaining. However, I'm wondering how this should integrate with other const-correctness efforts like `readability-non-const-parameter`? Also, I'm wondering how this check performs over a large code base like LLVM -- how chatty are the diagnostics, and how bad is the false positive rate (roughly)?



================
Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:32
+ *
+ * Handle = either a pointer or reference
+ * Value  = everything else (Type variable_name;)
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > Do you intend to support Obj-C object pointers as well?
> For now not, because I have no experience nor knowledge with Obj-C.
Okay, then please add a comment mentioning that they're explicitly not handled yet (perhaps with a FIXME).


================
Comment at: clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:147
+  // Example: `int i = 10`, `int i` (will be used if program is correct)
+  const auto LocalValDecl = varDecl(unless(anyOf(
+      isLocal(), hasInitializer(anything()), unless(ConstType),
----------------
JonasToth wrote:
> @aaron.ballman The change was not valid for some reason. I leave it like it is if thats ok with you.
That's.... really odd. I am fine leaving it as-is for this patch, but it would be good to understand why that code fails as it seems like a reasonable exposition.


================
Comment at: clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:183
+  // TODO Implement automatic code transformation to add the 'const'.
+  diag(Variable->getLocStart(), "variable %0 of type %1 can be declared const")
+      << Variable << Variable->getType();
----------------
Still missing the single quotes around `const` in the diagnostic.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444