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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
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)),
----------------
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > tiagoma wrote:
> > > > > JonasToth wrote:
> > > > > > Deactivating the transformations by default, as they are not ready for production yet.
> > > > > Ok. This got me off-guard. I would rather have this on.
> > > > the transformations were buggy and required a lot of fix-up. This might not be the case, but i would rather have it non-destructive by default because i think many people will throw this check at their code-base.
> > > > 
> > > > if the transformations are solid already we can activate it again (or maybe references only or so).
> > > Are we still intending to be conservative here? This defaults some of the Transform to 1 instead of 0, but that's different than what the documentation currently reads.
> > good question. given the current result of it being quiet solid we might have it on? But I am fine with it being off.
> > I am not sure if it would induce too much transformations to handle. I am unsure.
> Based on the test coverage we have, I think I'd be fine with it being on by default, but I do think the most conservative approach would be to be off by default. I don't have a strong opinion either way aside from "the documentation should match the code."
I matched the documentation to that its on by default. It is kinda what the check promises, so I think having it off is not user-friendly.
I removed the `Experimental` note for it as well. It seems to be "good enough" that it does not interfere with everyday programming and even complex things.
The different validation runs seem to agree somewhat (even though there are still issues).


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:40
+void define_locals(T np_arg0, T &np_arg1, int np_arg2) {
+  T np_local0 = 0;
+  int p_local1 = 42;
----------------
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > Are we being conservative here about the diagnostic? It seems like the instantiations in the TU would mean that this could be declared `const`.
> > Yes, the check ignores templates when matching. The reasoning behind this was: too much at once. TU-local templates should be transformable, but it must be ensured that all instantiations can be `const`. Probably the best solution would be that the template uses concepts and the concepts allow `const` at this place.
> > This is hopefully follow-up :)
> Okay, that sounds good to me!
Yes we are conservative. Once its a templated variable the analysis bails out. Non-templated variables in a template are analyzed though.

The 'check all instantiations' are not implemented, but there is already something that goes a little bit in that direction.
Line 118: `TemplateDiagnosticsCache.contains(Variable->getBeginLoc()))`, which is necessary to deduplicate emitted diagnostics in templates.
Similarly, TU-local templates could be checked all-together and if the variable could be `const` every single time, the diagnostic can be emitted.

But I feel that checking the concepts would be a much better approach to this. This would include the non-local templates as well. At the same time, most templates don't use them yet.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:114
+
+void casts() {
+  decltype(sizeof(void *)) p_local0 = 42;
----------------
aaron.ballman wrote:
> The test really doesn't have anything to do with casts, does it?
Thats correct :D Renamed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943