[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

Fri Mar 25 05:56:44 PDT 2022

aaron.ballman added a comment.

In D54943#3407789 <https://reviews.llvm.org/D54943#3407789>, @JonasToth wrote:

>> Sorry, for my own sanity, I've stopped reviewing C++ Core Guideline reviews for their diagnostic behavior until the guideline authors put effort into specifying realistic enforcement guidance.
>
> And how can we continue now? I fear that this is effectively the death of this check. :/
> Isn't the "make this const" good enough?

Our rule of thumb in clang-tidy when checking against a coding standard is that the coding standard is the law as to how the check should work (what it diagnoses, what it doesn't diagnose). When the coding standard gives guidance that can reasonably be handled another way, we give config options so that users can pick the behavior they need to enforce. The trouble is: the C++ Core Guidelines rarely have a useful enforcement to suggest and the rules themselves are often not sufficiently precise to tease out what to enforce. This puts the onus on *us* to decide what the behavior should be, which is inappropriate unless the guideline authors are involved in the discussions and reflect the decisions in the guidelines. To date, that has (almost?) never happened with any C++ Core Guideline checks.

In this specific case, the guideline being checked is https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on and its enforcement guidance is:

> Look to see if a variable is actually mutated, and flag it if not. Unfortunately, it might be impossible to detect when a non-const was not intended to vary (vs when it merely did not vary).

This is not useful enforcement guidance, so as a code reviewer, I have to spend *considerable* time trying to figure out what's reasonable and what's not. I've gone through this with basically each C++ Core Guideline check (many of them don't even TRY to give enforcement, like this: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enforcement-345), so this is not at all specific to you or something that's your fault. But that's why I no longer am willing to review C++ Core Guideline checks; they're a burden to support without the guideline authors active engagement, which has never materialized.

So how can we continue now is definitely a good question. I think there's plenty of utility in this check already, and so my recommendation would be to pull out all mentions of the C++ Core Guidelines (so we don't have to stick specifically to what they recommend) and AUTOSAR (see https://reviews.llvm.org/D112730#3332366 for details) and move the check to `misc-` (I don't see a better module for it at the moment as it's not really bugprone and it's not really readability, but I'm not strongly tied to which module it lives in). From there, I hope we can converge on the check MUCH faster because I think the "make this const" is good enough (and SUPER USEFUL). However, I don't know what your goals or requirements are (if you need this to adhere to the C++ Core Guidelines specifically), so another option is for the other reviewers to sign off on it; I won't actively block the addition of new C++ Core Guideline checks.

(Personally, I'm of the opinion we should pull the C++ Core Guidelines modules out of clang-tidy and distribute the existing checks amongst the other modules. Then we no longer have to worry about what the guidelines say, we can use them purely as inspiration for things we feel may be useful to check, but without tying ourselves to supporting their document. However, that's a decision which requires a far wider audience and is way outside the scope of this check.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943