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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
