Page MenuHomePhabricator

[NFC][clangd] cleaning up llvm-qualified-auto
Needs ReviewPublic

Authored by kuhnel on Nov 15 2021, 7:20 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This is a cleanup of all llvm-qualified-auto findings.
This patch was created by automatically applying the fixes from
clang-tidy.

Diff Detail

Event Timeline

kuhnel created this revision.Nov 15 2021, 7:20 AM
kuhnel published this revision for review.Nov 15 2021, 7:21 AM
kuhnel added subscribers: sammccall, adamcz, kbobyrev.

There is a reasonable style rule here, but:

  • the suggested fixes spell out const in ways we generally prefer not to. (And the rule doesn't require).
  • Strings are a special case. auto* for string literals makes no sense to me, the check should be modified to either accept auto or replace with const char*
clang-tools-extra/clangd/AST.cpp
441

We usually use auto* rather than const auto* for locals like this.

For the same reason we don't const locals: it adds more noise than it helps.
(And if it's critical we spell out these things, we probably shouldn't be using auto at all)

When this check was introduced, there was a discussion about this and they agreed to have the check stop flagging auto*, but if it sees auto it will still suggest the const. So the automatic fixes generally aren't what we want, we need to drop const.

clang-tools-extra/clangd/index/IndexAction.cpp
64

This is a particularly confusing type.
The original code is atypical for clangd, I'd suggest changing to auto* File

clang-tools-extra/clangd/index/dex/Iterator.cpp
209

const char* or auto or StringRef for strings. "Some kind of pointer" is technically correct but unhelpful.

Based on Sam's comments and our offline discussion, I would propose:

  1. abandon this patch
  2. start a discussion on the mailing list to disable this check for all of the llvm monorepo.

Other thoughts?

This check was written specifically for the LLVM monorepo, I doubt there'll be consensus to disable it.
https://reviews.llvm.org/D72217 has some previous discussion - some people want the consts spelled out, but we don't do this in clangd.
And I don't particularly think that would be the right thing to do: it's throwing out the baby with the bathwater.

I think we should rather:

  • fix the check to exempt strings (I don't think this would be terribly controversial, but I might be wrong)
  • land a version of this patch that doesn't add const in the places we don't normally use it

When trying to revert some of the changes, I just noticed there are a couple of if (const auto* ... in CodeComplete.cpp and AST.cpp (and maybe other files as well). So some folks seem to be using this right now. If we want to be consistent, we would have to remove these const as well.

I'm not sure what the right way forward would be.

@sammccall WDYT?

When trying to revert some of the changes, I just noticed there are a couple of if (const auto* ... in CodeComplete.cpp and AST.cpp (and maybe other files as well). So some folks seem to be using this right now. If we want to be consistent, we would have to remove these const as well.

I'm not sure what the right way forward would be.

@sammccall WDYT?

Yeah, we don't really have a consistent pattern for this after all.
In current code, where Foo is local and can be const:

  • we favor Foo over const Foo
  • we favor const Foo* over Foo* (the language often forces this)
  • auto *Foo vs const auto *Foo is a mix

Most of the time, I think the const is noise in contexts where auto is appropriate. But not all the time!
"This variable is intended for mutation" would be a useful signal, but marking every *other* variable as const is an ineffective and expensive way to do it. And we have no good way to keep that consistent.

So I'd suggest:

  • if applying bulk fixes like this and you don't want to look through case-by-case, prefer auto*
  • if a human has written const auto*, then there may or may not be a good reason. To "clean these up" would require two people to think about whether const communicates something important in each location, and I don't personally think that's a great use of time.

When trying to revert some of the changes, I just noticed there are a couple of if (const auto* ... in CodeComplete.cpp and AST.cpp (and maybe other files as well). So some folks seem to be using this right now. If we want to be consistent, we would have to remove these const as well.

Just in case it's news to anyone: (1) Theoretically, the compiler has enough information to tell you when it's deducing a cv-qualified type for a non-forwarding-reference auto; and personally I'd like such a warning and would go fix my code every time it fired. I like that const auto *p has the same "shape" as const Widget *p. That sometimes a non-const auto can secretly mean constWidget is weird and I don't like it.
(2) In template contexts, const auto * can be significant, because maybe in some instantiations the const doesn't matter and in others it does. https://quuxplusone.github.io/blog/2020/03/04/field-report-on-lifetime-extension/#true-positive-3-conditionally-redundant-lifetime-extension-in-template-code is related.

nridge added a subscriber: nridge.Nov 21 2021, 9:18 PM
kuhnel added a comment.Dec 2 2021, 8:19 AM

Looking at the documentation, this looks like a bug in the clang-tidy check:
https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-qualified-auto.html

The rule llvm-qualified-auto should not add a const qualifier.

Looking at the documentation, this looks like a bug in the clang-tidy check:
https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-qualified-auto.html

The rule llvm-qualified-auto should not add a const qualifier.

I don't see how that is a bug: the docs specially cover the const auto * conversion in the first example by drawing the line between observe() (non-mutating function) and change() (possibly mutating function).

Looking at the documentation, this looks like a bug in the clang-tidy check:
https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-qualified-auto.html

The rule llvm-qualified-auto should not add a const qualifier.

I don't see how that is a bug: the docs specially cover the const auto * conversion in the first example by drawing the line between observe() (non-mutating function) and change() (possibly mutating function).

Sorry I should have been more precise here: AddConstToQualified allows to configure if we want to have const added or not. The default for that rule is true. However the last sentence on that page says:

Note in the LLVM alias, the default value is false.

So my understanding of this is: With the LLVM-rules we're using, const should not be added.

kuhnel updated this revision to Diff 393890.Dec 13 2021, 7:22 AM

reverted the const as advised in Sam's comment

kuhnel updated this revision to Diff 393892.Dec 13 2021, 7:25 AM

fixed more const ClangdTests.cpp

kuhnel updated this revision to Diff 393894.Dec 13 2021, 7:36 AM

more reverts of consts

kuhnel marked 3 inline comments as done.Dec 13 2021, 7:37 AM
kuhnel added inline comments.
clang-tools-extra/clangd/unittests/HoverTests.cpp
2550

while clang-tidy added a const here, I would keep it for consistency with the next line...

TODO: check for remaining const then submit.

clang-tools-extra/clangd/unittests/FileIndexTests.cpp
252

remove const here