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

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1feb7af04688: [clangd] support expanding `decltype(expr)` 
(authored by v1nh1shungry, committed by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D141226?vs=488297=488841#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -130,7 +130,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could 

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

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry marked 2 inline comments as done.
v1nh1shungry added a comment.

Thank you for reviewing and giving guidance!

> Do you have commit access?

I don't. If this patch is okay to land, could you please help me commit it? 
Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 488297.
v1nh1shungry added a comment.

modify the comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type"));
+  // function references should not be replaced
+  EXPECT_THAT(apply("au^to  = ns::Func;"),
+  StartsWith("fail: Could not expand 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:151
+
+  // We shouldn't replace types like function and array, the commonality 
between
+  // these cases is that they use C-style declarator syntax that may have 
chunks

sammccall wrote:
> Say why rather than what, e.g.
> 
> ```
> Some types aren't written as single chunks of text, e.g:
>   auto fptr =  // auto is void(*)()
> ==>
>   void (*fptr)();
> 
> Replacing these requires examining the declarator, we don't support it yet.
> ```
oops, that should be `void (*fptr)() = ` of course..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LGTM with a comment nit, thank you!
Do you have commit access?




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:151
+
+  // We shouldn't replace types like function and array, the commonality 
between
+  // these cases is that they use C-style declarator syntax that may have 
chunks

Say why rather than what, e.g.

```
Some types aren't written as single chunks of text, e.g:
  auto fptr =  // auto is void(*)()
==>
  void (*fptr)();

Replacing these requires examining the declarator, we don't support it yet.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 488265.
v1nh1shungry added a comment.

address the comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type"));
+  // function references should not be replaced
+  EXPECT_THAT(apply("au^to  = ns::Func;"),
+  StartsWith("fail: Could not 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139
 
-  // if it's a lambda expression, return an error message
-  if (isa(*DeducedType) &&
-  cast(*DeducedType)->getDecl()->isLambda()) {
-return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template 

v1nh1shungry wrote:
> sammccall wrote:
> > v1nh1shungry wrote:
> > > 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 ` > > type>`. Though I haven't looked into this, would you mind giving some 
> > > suggestions about it?
> > Ah, there are three subtly different concepts here:
> >  - is this type usefully printable? There are various reasons why it may 
> > not be, it can contain DependentTy, or a canonical template parameter 
> > (``), or a lambda.
> >  - is this type dependent in the language sense - an example is some type 
> > parameter `T`. This may or may not be usefully printable. (e.g. try 
> > `template std::vector y;` and hover on y)
> >  - is this type specifically `DependentTy`, the placeholder dependent type 
> > which we have no information about. This is never usefully printable: 
> > prints as ``
> > 
> > As a heuristic, I'm happy with saying dependent types (in the language 
> > sense) are likely not to print usefully, so don't replace them. But the 
> > comment should say so (this is a heuristic for unprintable rather than an 
> > end in itself), and probably give examples.
> Hmm, do you mean it's okay to refuse to replace dependent types but I should 
> leave a comment saying the reason is that they are likely not to print 
> usefully? Sorry if I misunderstood anything! I'm really not a good reader :(
Yeah, exactly!



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");

v1nh1shungry wrote:
> sammccall wrote:
> > 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.
> TBH, I'm confused about `printType()` and its placeholder `DECLARATOR_ID`. Is 
> the code your offered above can be used directly?
> 
> If so, I had a try on it and it did refuse to replace types like function and 
> array. But it would give an error message saying "Could not expand 
> non-contiguous type const char (_ID)[6]". Is this okay? I mean 
> isn't the "DECLARATOR_ID" in the message a bit confusing?
That was the idea, but I think you're right the error message is confusing.

We need to strike a balance between being precise, being easy to understand, 
and being reasonable to maintain.

I think it's probably best to be a bit vague here rather than using precise but 
obscure language, maybe "could not expand type that isn't a simple string".
Whether to leave out the actual type, include it with DECLARATOR_ID in it, or 
call printType() again with an empty placeholder is up to you - I don't think 
any of those are too confusing in context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139
 
-  // if it's a lambda expression, return an error message
-  if (isa(*DeducedType) &&
-  cast(*DeducedType)->getDecl()->isLambda()) {
-return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template 

sammccall wrote:
> v1nh1shungry wrote:
> > 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 ``. 
> > Though I haven't looked into this, would you mind giving some suggestions 
> > about it?
> Ah, there are three subtly different concepts here:
>  - is this type usefully printable? There are various reasons why it may not 
> be, it can contain DependentTy, or a canonical template parameter 
> (``), or a lambda.
>  - is this type dependent in the language sense - an example is some type 
> parameter `T`. This may or may not be usefully printable. (e.g. try 
> `template std::vector y;` and hover on y)
>  - is this type specifically `DependentTy`, the placeholder dependent type 
> which we have no information about. This is never usefully printable: prints 
> as ``
> 
> As a heuristic, I'm happy with saying dependent types (in the language sense) 
> are likely not to print usefully, so don't replace them. But the comment 
> should say so (this is a heuristic for unprintable rather than an end in 
> itself), and probably give examples.
Hmm, do you mean it's okay to refuse to replace dependent types but I should 
leave a comment saying the reason is that they are likely not to print 
usefully? Sorry if I misunderstood anything! I'm really not a good reader :(



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");

sammccall wrote:
> 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.
TBH, I'm confused about `printType()` and its placeholder `DECLARATOR_ID`. Is 
the code your offered above can be used directly?

If so, I had a try on it and it did refuse to replace types like function and 
array. But it would give an error message saying "Could not expand 
non-contiguous type const char (_ID)[6]". Is this okay? I mean isn't 
the "DECLARATOR_ID" in the message a bit confusing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139
 
-  // if it's a lambda expression, return an error message
-  if (isa(*DeducedType) &&
-  cast(*DeducedType)->getDecl()->isLambda()) {
-return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template 

v1nh1shungry wrote:
> 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 ``. 
> Though I haven't looked into this, would you mind giving some suggestions 
> about it?
Ah, there are three subtly different concepts here:
 - is this type usefully printable? There are various reasons why it may not 
be, it can contain DependentTy, or a canonical template parameter 
(``), or a lambda.
 - is this type dependent in the language sense - an example is some type 
parameter `T`. This may or may not be usefully printable. (e.g. try 
`template std::vector y;` and hover on y)
 - is this type specifically `DependentTy`, the placeholder dependent type 
which we have no information about. This is never usefully printable: prints as 
``

As a heuristic, I'm happy with saying dependent types (in the language sense) 
are likely not to print usefully, so don't replace them. But the comment should 
say so (this is a heuristic for unprintable rather than an end in itself), and 
probably give examples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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(*DeducedType) &&
-  cast(*DeducedType)->getDecl()->isLambda()) {
-return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template 

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 ``. 
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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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()) {
+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(*DeducedType) &&
-  cast(*DeducedType)->getDecl()->isLambda()) {
-return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template 

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry updated this revision to Diff 487704.
v1nh1shungry added a comment.

avoid expanding pointer to an array


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp
  clang-tools-extra/clangd/test/check-fail.test
  clang-tools-extra/clangd/test/check-lines.test
  clang-tools-extra/clangd/test/check.test
  clang-tools-extra/clangd/test/code-action-request.test
  clang-tools-extra/clangd/test/request-reply.test
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang/docs/tools/clang-formatted-files.txt
  llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
  llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn
@@ -129,7 +129,7 @@
 "tweaks/DumpASTTests.cpp",
 "tweaks/DumpRecordLayoutTests.cpp",
 "tweaks/DumpSymbolTests.cpp",
-"tweaks/ExpandAutoTypeTests.cpp",
+"tweaks/ExpandDeducedTypeTests.cpp",
 "tweaks/ExpandMacroTests.cpp",
 "tweaks/ExtractFunctionTests.cpp",
 "tweaks/ExtractVariableTests.cpp",
Index: llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clangd/refactor/tweaks/BUILD.gn
@@ -18,7 +18,7 @@
 "DefineInline.cpp",
 "DefineOutline.cpp",
 "DumpAST.cpp",
-"ExpandAutoType.cpp",
+"ExpandDeducedType.cpp",
 "ExpandMacro.cpp",
 "ExtractFunction.cpp",
 "ExtractVariable.cpp",
Index: clang/docs/tools/clang-formatted-files.txt
===
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -1611,7 +1611,7 @@
 clang-tools-extra/clangd/unittests/tweaks/DumpASTTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpRecordLayoutTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/DumpSymbolTests.cpp
-clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
+clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExpandMacroTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp
 clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Index: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
===
--- clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
+++ clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
@@ -25,9 +25,9 @@
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
-// TWEAK_TEST(ExpandAutoType);
+// TWEAK_TEST(ExpandDeducedType);
 //
-// TEST_F(ExpandAutoTypeTest, ShortensTypes) {
+// TEST_F(ExpandDeducedTypeTest, ShortensTypes) {
 //   Header = R"cpp(
 // namespace foo { template class X{}; }
 // using namespace foo;
Index: clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/ExpandDeducedTypeTests.cpp
@@ -1,4 +1,4 @@
-//===-- ExpandAutoTypeTests.cpp -*- C++ -*-===//
+//===-- ExpandDeducedTypeTests.cpp --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -16,9 +16,9 @@
 namespace clangd {
 namespace {
 
-TWEAK_TEST(ExpandAutoType);
+TWEAK_TEST(ExpandDeducedType);
 
-TEST_F(ExpandAutoTypeTest, Test) {
+TEST_F(ExpandDeducedTypeTest, Test) {
   Header = R"cpp(
 namespace ns {
   struct Class {
@@ -50,7 +50,10 @@
   StartsWith("fail: Could not deduce type for 'auto' type"));
   // function pointers should not be replaced
   EXPECT_THAT(apply("au^to x = ::Func;"),
-  StartsWith("fail: Could not expand type of function pointer"));
+  StartsWith("fail: Could not expand type of function"));
+  // function references should not be replaced
+  EXPECT_THAT(apply("au^to  = ns::Func;"),
+