[PATCH] D73548: [clang-tidy] Added option for disabling const qualifiers in readability-qualified-auto

[Nathan James via Phabricator via cfe-commits]

njames93 marked 7 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst:41
+   auto *const Bar = cast<const int *>(Baz2);
+   auto *volatile FooBar = cast<int *>(Baz3);
+
----------------
Quuxplusone wrote:
> Is it worth adding an example of a double pointer?
> 
>     auto BarN = cast<int **>(FooN);
> 
> Does that become `auto*` or `auto**` (and why)? My wild guess is that it becomes `auto*` (and because nobody cares about double pointers), but I could be wrong.
Double pointers are just resolved to `auto *`.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst:64
+
+   Otherwise it will get be transformed into:
+
----------------
Quuxplusone wrote:
> s/Will be/will be/
> s/will get be/will be/
> 
> In the preceding section you give an example with `volatile`. How about here? What happens with
> 
>     auto *volatile Foo3 = cast<const int *>(Bar3);
>     auto *Foo4 = cast<volatile int *>(Bar4);
> 
> Do they become
> 
>     const auto *volatile Foo3 = cast<const int *>(Bar3);
>     volatile auto *Foo4 = cast<volatile int *>(Bar4);
> 
> as I would expect?
Good spot on the Will/be/get/(whatever I tried to type)

the check keeps local volatile qualifiers, but it wont add volatile pointer (or ref) qualifiers even if the deduced type is a volatile pointer. In the first patch we came to the conclusion not to add them.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst:69
+   const auto *Foo1 = cast<const int *>(Bar1);
+   const auto &Foo2 = cast<const int &>(Bar2);
+
----------------
Quuxplusone wrote:
> How does this option interact with
> 
>     const int range[10];
>     for (auto&& elt : range)
> 
> Does it (incorrectly IMHO) make that `const auto&& elt`, or (correctly IMHO) `const auto& elt`, or (also correctly) leave it as [`auto&&`, which Always Works](https://quuxplusone.github.io/blog/2018/12/15/autorefref-always-works/)?
RValueReferences are ignored


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/llvm-qualified-auto.cpp:20
+  auto &NakedRef = *getIntPtr();
+  auto &NakedConstRef = *getCIntPtr();
+}
----------------
Quuxplusone wrote:
> It is worth adding one test case to show that the LLVM alias does not gratuitously //remove// `const` from declarations:
> 
>     auto const &ExtendedConstPtr = getCIntPtr();
>     // becomes
>     auto *const &ExtendedConstPtr = getCIntPtr();
That test case looks malformed as the function getCIntPtr() doesn't return a reference. as for removing const etc those cases are all in the `readability-qualified-auto.cpp` test file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73548