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