[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:
> > 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.


================
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:
> 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 :)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:120
+
+// taken from http://www.cplusplus.com/reference/type_traits/integral_constant/
+template <typename T, T v>
----------------
aaron.ballman wrote:
> This is a problem -- I see no license on that site, but I do see "All rights reserved". I don't think we should copy from there.
True. I will try to copy&paste from libc++ instead and remove the reference :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943