[PATCH] D62855: Implementation of auto type expansion.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry about the delay here. Happy to chat offline if I'm being confusing.



================
Comment at: clang-tools-extra/clangd/AST.cpp:172
 
+namespace {
+/// Computes the deduced type at a given location by visiting the relevant
----------------
It looks like this has been moved from somewhere (at least code looks familiar) but isn't deleted anywhere. (The code in XRefs is touched but doesn't seem to use this). Is there a reason we can't reuse one copy?




================
Comment at: clang-tools-extra/clangd/AST.h:72
+// TODO: add documentation
+llvm::Optional<QualType> getDeductedType(SourceLocation SearchedLocation, ASTContext &AST);
+
----------------
nit: deduced
(also docs! In particular the fact that the SearchedLocation should point to exactly an `auto`, `decltype` etc token)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:76
+std::string
+ExpandAutoType::shortenNamespace(const llvm::StringRef &OriginalName,
+                                 const llvm::StringRef &CurrentNamespace) {
----------------
This is nice. We might want to move it to SourceCode.h, somewhere near SplitQualifiedName. Then we'd generally unit test all the cases in SourceCodeTests, and we only have to smoke test the shortening here.

One limitation is that it only handles shortening the qualifier on the name itself, but not other parts of a printed type. e.g. `namespace ns { struct S(); auto* x = new std::vector<S>(); }` will expand to `std::vector<ns::S>`, not `std::vector<S>`. To fully solve this we may want to modify PrintingPolicy at some point, though we could probably get a long way by searching for chunks of text inside printed types that look like qualified names.

I think it might more clearly communicate the limitations if this function operated on the scope part only, e.g. `printScope("clang::clangd::", "clang::") --> "clangd::"`.

In the general case, `CurrentNamespace` should be a vector, because there can be others (introduced by using-declarations). Fine to leave this as a fixme if it's hard to do here, though.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:76
+std::string
+ExpandAutoType::shortenNamespace(const llvm::StringRef &OriginalName,
+                                 const llvm::StringRef &CurrentNamespace) {
----------------
sammccall wrote:
> This is nice. We might want to move it to SourceCode.h, somewhere near SplitQualifiedName. Then we'd generally unit test all the cases in SourceCodeTests, and we only have to smoke test the shortening here.
> 
> One limitation is that it only handles shortening the qualifier on the name itself, but not other parts of a printed type. e.g. `namespace ns { struct S(); auto* x = new std::vector<S>(); }` will expand to `std::vector<ns::S>`, not `std::vector<S>`. To fully solve this we may want to modify PrintingPolicy at some point, though we could probably get a long way by searching for chunks of text inside printed types that look like qualified names.
> 
> I think it might more clearly communicate the limitations if this function operated on the scope part only, e.g. `printScope("clang::clangd::", "clang::") --> "clangd::"`.
> 
> In the general case, `CurrentNamespace` should be a vector, because there can be others (introduced by using-declarations). Fine to leave this as a fixme if it's hard to do here, though.
StringRefs are passed by value. They're just shorthand for a char* + length.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.h:1
+//===--- Tweak.h -------------------------------------------------*- C++-*-===//
+//
----------------
update filename

(these headers are so silly :-()


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.h:21
+///    std::string x = Something();
+///    ^^^^^^^^^^^
+class ExpandAutoType : public Tweak {
----------------
Maybe add a FIXME to handle decltype here too? It's pretty similar (though rarer).


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:221
 
+TEST(TweakTest, ExpandAutoType) {
+  llvm::StringLiteral ID = "ExpandAutoType";
----------------
can you add testcases for:
 - unknown types in a template `template <typename T> void x() { auto y = T::z(); }`
 - broken code `auto x = doesnt_exist();`
 - lambda `auto x = []{};`
 - inline/anon namespace: `inline namespace x { namespace { struct S; } } auto y = S();` should insert "S" not "x::S" or "(anonymous)::S".
 - local class: `namespace x { void y() { struct S{}; auto z = S(); } }` (should insert "S")

(it's ok if the behavior is bad in these cases, we can fix that later. Ideal in first 3 is no change)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62855