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<DecltypeTypeLoc>()) {
+ 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<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>
----------------
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
