[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this!

There are a couple of separate logical changes here - I don't think it's a problem per se, but it does mean it takes a bit longer to get the good + obvious stuff reviewed and landed.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:57
+std::string ExpandDeducedType::title() const {
+  return IsAutoType ? "Expand auto type" : "Expand decltype";
+}
----------------
Maybe "Replace with deduced type" would be clearer for both cases?

Then we don't need to track IsAutoType.
(I have no objection with spending a bit of code to provide better text, but given that the cursor is either on top of "auto" or "decltype" I don't think we're actually adding anything by echoing it back)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:119
+      if (auto DTTL = TypeNode->getAs<DecltypeTypeLoc>()) {
+        if (!isDeducedAsLambda(Node, DTTL.getBeginLoc()))
+          Range = DTTL.getSourceRange();
----------------
isDeducedAsLambda is detecting the specific pattern `auto x = [&} { ... };`, which doesn't occur with decltype.

On the other hand I suspect the `DecltypeTypeLoc` for `decltype(some_lambda)` doesn't suffer from the same issue as the `AutoTypeLoc` in a declaration, and we can simply call getUnderlyingType().

So I think you can simply factor out `isLambda(QualType)`  from `isDeducedAsLambda`, and call `isLambda(DTTL.getType()->getUnderlyingType())` here.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139
 
-  // if it's a lambda expression, return an error message
-  if (isa<RecordType>(*DeducedType) &&
-      cast<RecordType>(*DeducedType)->getDecl()->isLambda()) {
-    return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template <class T>
----------------
why not? replacing this with `T` seems perfectly reasonable.
(The fact that we don't do this with `auto t = T{}` is just because we're hitting a limitation of clang's AST)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:149
+  //   int arr[10];
+  //   decltype(auto) foobar = arr;
+  //   ^^^^^^^^^^^^^^ is `int[10]`
----------------
these are unrelated to the decltype change (`decltype(auto)` is an AutoType rather than a DecltypeType and already works/has this bug today).

In future it's best to split unrelated changes into different patches/a patch sequence, though this is simple enough it's probably OK.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:151
+  //   ^^^^^^^^^^^^^^ is `int[10]`
+  if ((*DeducedType)->isArrayType())
+    return error("Could not expand type of array");
----------------
the commonality between these cases you're excluding (and the function types below) is that they use C-style declarator syntax that may have chunks following the declarator-id. Specifically:
 - array, function, and paren types always have chunks following the declarator
 - pointer and reference types compose inside-out so their pointee types may contain trailing chunks

If we want to make some attempt to detect more cases, we should pull out a function here and do it properly, something like: `bool hasTrailingChunks(QualType)` which calls recursively for pointertype etc.

But there's a cheaper way: when we call `printType()` below, we can optionally specify the declarator-id. Then we can simply check whether it's at the end of the string:

```
std::string PrettyDeclarator = printType(..., "DECLARATOR_ID");
llvm::StringRef PrettyType = PrettyDeclarator;
if (!PrettyType.consume_back("DECLARATOR_ID"))
  return error("could not expand non-contiguous type {0}", PrettyType);
PrettyType = PrettyType.rtrim();
```

This feels slightly hacky, but it's direct and simple and we shouldn't miss a case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226