[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 15 05:28:34 PST 2021
aaron.ballman added a comment.
In D54943#3194647 <https://reviews.llvm.org/D54943#3194647>, @JonasToth wrote:
> In D54943#3194643 <https://reviews.llvm.org/D54943#3194643>, @aaron.ballman wrote:
>
>> Thanks for pinging on this, it had fallen off my radar! @JonasToth -- did you ever get the chance to talk with @cjdb about the overlap mentioned in https://reviews.llvm.org/D54943#2950100? (The other review also seems to have stalled out, so I'm not certain where things are at.)
>
> We talked only short in the review, but I am not sure if there is such a big overlap between the checks. The other check looks at arguments, which is something this check currently skips completely.
> I think, this check should continue to avoid this analysis, too.
That's what sort of worries me though -- a function parameter is notionally a local variable, so the fact that one check tries to solve one half of the problem and the other check tries to solve the other half feels unclean to me. I'm fine with this check ignoring parameters for now, but I think there should only be one check for "make this const if it can be made so".
> The underlying `ExprMutAnalyzer` is reusable (I think so, if not that should be fixed). But my current understanding is, that the argument checker already knows the values are `const`, because it matches on `const &` parameters.
> IMHO keeping the checks independent would be better. Otherwise a future combined check can be aliased with specific configurations.
I think surfacing the checks as separate checks makes a lot of sense (once is about C++ Core Guideline conformance and the other is about performance), but I'm not sure I understand why the implementations should be different yet.
That said, I think this particular check is more mature and closer to landing than the other one. Based on the amount of effort in this patch already, unless there are strong objections, I think we should base the "make this const if it can be made so" checks on this one.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:49-51
+ // The rules for C and 'const' are different and incompatible for this check.
+ if (!getLangOpts().CPlusPlus)
+ return;
----------------
The new-fangled way to do this is to implement an override for `isLanguageVersionSupported()` (as in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/abseil/DurationDivisionCheck.h#L26)
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:111-116
+ /// If the variable was declared in a template it might be analyzed multiple
+ /// times. Only one of those instantiations shall emit a warning. NOTE: This
+ /// shall only deduplicate warnings for variables that are not instantiation
+ /// dependent. Variables like 'int x = 42;' in a template that can become
+ /// const emit multiple warnings otherwise.
+ const bool IsNormalVariableInTemplate = Function->isTemplateInstantiation();
----------------
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:168
+ *Variable, *Result.Context, DeclSpec::TQ_const,
+ QualifierTarget::Value, QualifierPolicy::Right)) {
+ Diag << *Fix;
----------------
JonasToth wrote:
> tiagoma wrote:
> > The core guidelines suggests the use of west const. Could this be made the default?
> > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rl-const
> https://reviews.llvm.org/D69764
>
>
> I was hoping for this feature to handle this instead.
> It is extremely complicated to figure out where to insert the `const`, because pointer, pointee, value, templates, macros and so on and so forth.
> I implemented it such that it is always correct (the right side) and hoped that clang-format can fix it up afterwards (`clang-tidy -fix -format`)
That's the correct approach -- we don't try to apply style choices to fixes in clang-tidy unless they're uncontentious (removing extra whitespace kinda stuff); we expect the user to run clang-format to fix those sort of things up.
================
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:
> 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.
================
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;
----------------
Are we being conservative here about the diagnostic? It seems like the instantiations in the TU would mean that this could be declared `const`.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:107-108
+ int *np_local3[2] = {&np_local0[0], &np_local0[1]};
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local3' of type 'int *[2]' can be declared 'const'
+ // CHECK-FIXES: const
+ for (int *non_const_ptr : np_local3) {
----------------
This is a false positive. Making `np_local3` be `const` will break the code: https://godbolt.org/z/EExKfsW4G
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:114
+
+void casts() {
+ decltype(sizeof(void *)) p_local0 = 42;
----------------
The test really doesn't have anything to do with casts, does it?
================
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>
----------------
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.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:164-167
+ TMPClass<double> p_local1;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'TMPClass<double>' can be declared 'const'
+ // CHECK-FIXES: const
+ p_local1.alwaysConst();
----------------
Can you also add a test:
```
TMPClass<double> p_local2; // Don't attempt to make this const
p_local2.sometimesConst();
```
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:13
+bool global;
+char np_global = 0; // globals can't be known to be const
+
----------------
It'd be handy to show how we handle a static global, which can be known to be const technically.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:17
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
----------------
Similarly, showing anonymous namespace variables would help, as those are like a static global and can also theoretically known to be const. (Note, I don't insist on catching those cases! Just would like test coverage with some comments on what to expect + docs if it seems important to tell users.)
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:555-574
+// taken from http://www.cplusplus.com/reference/type_traits/integral_constant/
+template <typename T, T v>
+struct integral_constant {
+ static constexpr T value = v;
+ using value_type = T;
+ using type = integral_constant<T, v>;
+ constexpr operator T() { return v; }
----------------
Same comments here as above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54943/new/
https://reviews.llvm.org/D54943
More information about the cfe-commits
mailing list