[PATCH] D54943: [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.h:31
+        WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
+        TransformValues(Options.get("TransformValues", 1)),
+        TransformReferences(Options.get("TransformReferences", 1)),
----------------
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."


================
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;
----------------
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!


================
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>
----------------
JonasToth wrote:
> 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 :)
Thank you. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943