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

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added a comment.

Thank you for reviewing and giving suggestions!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:57
+std::string ExpandDeducedType::title() const {
+  return IsAutoType ? "Expand auto type" : "Expand decltype";
+}
----------------
sammccall wrote:
> 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)
I think your suggestion is better. Thanks!


================
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>
----------------
sammccall wrote:
> 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)
I'd like it too. But the `printType()` would give out a `<dependent type>`. Though I haven't looked into this, would you mind giving some suggestions about it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226