[PATCH] D62855: [clangd] Implementation of auto type expansion.

Tue Jul 9 08:48:32 PDT 2019

kuhnel added inline comments.

================
Comment at: clang-tools-extra/clangd/AST.cpp:172

+namespace {
+/// Computes the deduced type at a given location by visiting the relevant
----------------
sammccall wrote:
> 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?
> 
> 
Ah interesting. It got messed up during rebase and someone implemented a similar function in XRefs.cpp in the mean time. So I'll just move over to their implementation. And remove the changes to AST...

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:32
+  CachedLocation = findAutoType(Node);
+  return CachedLocation != llvm::None;
+}
----------------
sammccall wrote:
> && `!CachedLocation->getTypePtr()->getDeducedType()->isNull()`?
> 
> to avoid triggering in e.g. dependent code like
> ```
> template <typename T> void f() {
> auto X = T::foo();
> }
> ```
I added the new testcase and if fails with and without the added condition.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:39
+  llvm::Optional<clang::QualType> DeductedType =
+      getDeducedType(Inputs.AST, CachedLocation->getBeginLoc());
+
----------------
sammccall wrote:
> is this ever not just `CachedLocation->getTypePtr()->getDeducedType()`?
There is some difference, because quite a few tests fail, wenn I use your call. --> Staying with current implementation

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:47
+  // if it's a lambda expression, return an error message
+  if (isa<RecordType>(*DeductedType) and
+      dyn_cast<RecordType>(*DeductedType)->getDecl()->isLambda()) {
----------------
sammccall wrote:
> again, this is actually a cheap test (if we don't need to use the deduced type visitor), we can lift it into prepare
Then I would have to move the call to `getDeducedType(...)` also to `prepare`....

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:53
+
+  SourceRange OriginalRange(CachedLocation->getBeginLoc(),
+                            CachedLocation->getEndLoc());
----------------
sammccall wrote:
> nit: this is just CachedLocation->getSourceRange()
correct.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp:68
+const llvm::Optional<AutoTypeLoc>
+ExpandAutoType::findAutoType(const SelectionTree::Node* StartNode) {
+  auto Node = StartNode;
----------------
sammccall wrote:
> I'm confused about this - how can the user select the child of an AutoTypeLoc rather than the AutoTypeLoc itself? i.e. do we need the loop?
At first I understood that we would have to walk the AST, but it seems we don't have to...

================
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:
> 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.
1) added documentation on that.

2) In general it would be nicer to do these operations on a some model and not on bare strings. We could probably recursively search for more namespaces inside angle brackets <>...

================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:221

+TEST(TweakTest, ExpandAutoType) {
+  llvm::StringLiteral ID = "ExpandAutoType";
----------------
sammccall wrote:
> 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)
Did not add tests, as the code does not yet support this:
* `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")

This requires some changes on how I identify the namespaces. I would do that in a second version/increment of the feature.

Working:
* unknown types in a template template <typename T> void x() { auto y = T::z(); }
* broken code auto x = doesnt_exist();
* lambda auto x = []{};

================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:396
+  )cpp";
+  checkTransform(ID, Input, Output);
+}
----------------
sammccall wrote:
> can we have one for function pointers?
> 
> ```
> int foo();
> auto x = &foo;
> ```
> 
> The correct replacement would be `int (*x)() = &foo;` but I think we should just aim to produce no edits in such cases.
You're an infinite oracle of strange C++ corner cases ;)
I added a test and another check for this case...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62855