[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Whats left from my personal todo is adjusting the documentation.
I think then this check can work as linter and if you want as fixer as well, but this has to be enable explicitly.

I think that fixing still has more value than harm, e.g. in your IDE/editor. The most annyoing part is that `const` may be applied multiple times. But you can delete the superfluous consts and then you are done,  the variable will not be considered anymore.

Did I miss important things?



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:46
+
+void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ConstType = hasType(isConstQualified());
----------------
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > aaron.ballman wrote:
> > > > > Should this check only fire in C++? I'm sort of on the fence. It's a C++ core guideline, so it stands to reason it should be disabled for C code. But const-correctness is a thing in C too. WDYT?
> > > > I do not know all the subtle differences between C and C++ here. Judging from this: https://stackoverflow.com/questions/5248571/is-there-const-in-c the current form would not be correct for C for pointers.
> > > > The assumptions of this check and especially the transformations are done for C++. I dont see a reason why the value-semantic would not apply for both.
> > > > 
> > > > Maybe there is a way to make the code compatible for both languages. The easiest solution would probably to not do the 'pointer-as-value' transformation. This is not that relevant as a feature anyway. I expect not nearly as much usage of this option as for the others.
> > > > 
> > > > In the end of the day i would like to support both C and C++. Right now it is untested and especially the transformation might break the code. It should run on both languages though, as there is no language checking.
> > > > I will add some real world C code-bases for the transformation testing and see what happens :)
> > > > Judging from this: https://stackoverflow.com/questions/5248571/is-there-const-in-c the current form would not be correct for C for pointers.
> > > 
> > > Sure, there may be additional changes needed to support the other oddities of C. I was asking more at the high level.
> > > 
> > > > In the end of the day i would like to support both C and C++. Right now it is untested and especially the transformation might break the code. 
> > > 
> > > Another option is for us to restrict the check in its current form to just C++ and then expose a C check from it in a follow-up (likely under a different module than cppcodeguidelines). For instance, CERT has a recommendation (not a rule) about this for C (https://wiki.sei.cmu.edu/confluence/display/c/DCL00-C.+Const-qualify+immutable+objects) and I would not be surprised to find it in other coding standards as well.
> > Another const check is probably better. The clang-tidy part should be minimal effort. I think there isn't alot of duplication either.
> > We could still think of proper configurability of this check and alias it into other modules with proper defaults for C.
> Okay, I'm sold -- can you not register the check unless in C++ mode? We can add a C-specific check later.
Register only for C++


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:31
+        WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
+        TransformValues(Options.get("TransformValues", 1)),
+        TransformReferences(Options.get("TransformReferences", 1)),
----------------
Deactivating the transformations by default, as they are not ready for production yet.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:830
+
+void complex_usage() {
+  int np_local0 = 42;
----------------
aaron.ballman wrote:
> Here's another terrible one to try -- can you make sure we don't try to make something const when the variable is evaluated in a context where it's usually not:
> ```
> int N = 1; // Can't make N 'const' because VLAs make everything awful
> sizeof(int[++N]);
> ```
Already covered ;) I was afraid :D


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54943/new/

https://reviews.llvm.org/D54943