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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
