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<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>
----------------
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 `<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 :(
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 (&DECLARATOR_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
