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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:46
+
+void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ConstType = hasType(isConstQualified());
----------------
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:830
+
+void complex_usage() {
+  int np_local0 = 42;
----------------
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]);
```


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:923
+
+#if 0
+template <typename T>
----------------
Rather than comment out the code, I think it makes more sense to leave the code here, CHECK for the existing diagnostics, but add a FIXME comment to any diagnostics (or lack of diagnostics) that are incorrect. It makes it more clear what the current behavior is and what we want it to eventually be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943