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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits