[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<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:
> 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 `<dependent 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 (`<template-param-0-0>`), 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<class X> std::vector<X> 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 `<dependent type>`
> 
> 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 (&DECLARATOR_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