[PATCH] D72217: [clang-tidy] Added readability-qualified-auto check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72217#1844262 <https://reviews.llvm.org/D72217#1844262>, @sammccall wrote:

> In D72217#1844204 <https://reviews.llvm.org/D72217#1844204>, @njames93 wrote:
>
> > https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto
> >
> >   // Copy pointers, but make it clear that they're pointers.
> >   for (const auto *Ptr : Container) { observe(*Ptr); }
> >   for (auto *Ptr : Container) { Ptr->change(); }
> >
> >
> > This is the reasoning behind putting the const qualifier in the check. If people feel that this isn't quite enough to justify the const qualifier I'll submit a follow up. The general consensus though is that you should mark auto pointers as const if they don't need to change to express intent
>
>
> The text/rule there is explicitly about avoiding/clarifying copies - the examples indeed use 'const' but AFAICT the "don't copy" reasoning only applies to including *&.
>
> FWIW I think const here is often noise, particularly in AST-walking code where you're traversing an edge from an X* to a Y* - the latter will be const if the former is, and I care at API boundaries but not in between. (It's usually a meaningless distinction - e.g. we're just reading but it's a non-const pointer because RecursiveASTVisitor isn't const-friendly).
>
> So while spelling const is often helpful, we shouldn't (and don't) require it, and the current config of this check is too intrusive.


Oddly enough, there were several long-time reviewers (including myself) who firmly believed we did document this requirement at one point in time, but we couldn't find evidence of it when we started digging harder. We've been giving this review feedback consistently for many years, so "we don't require it" may not be entirely accurate. We may not have written down a requirement for it, but I've *definitely* heard that we should not be deducing qualifiers and I agree with that sentiment. It makes the code harder to understand as a reviewer because there's no way to tell whether code is mutating the pointer/reference or not without further digging (and it changes where code dispatches to, so it's important information for readers of the code to have).

I'm fine making this into an option, but my preference be that the option is on by default for the readability checker. For the LLVM checker, maybe we default the option to off and argue about changing the coding standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72217