sammccall added a comment.
Ok, this looks pretty good to me! Just nits
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:170
+ if (E->hasPlaceholderType(BuiltinType::PseudoObject)) {
+ if (const auto *PR = dyn_cast<ObjCPropertyRefExpr>(E)) {
+ if (PR->isMessagingSetter()) {
----------------
dgoldman wrote:
> sammccall wrote:
> > E->getObjCProperty()?
> Hmm I'm not sure when that would be useful looking at the logic there, can
> you give an example just to make sure I understand what it would handle?
it's similar to the cast you have, but in addition to handling `foo.bar` it
handles parens like `(foo.bar)`, commas like `(0, foo.bar)` and casts. All
together I think this covers more cases where a pseudo-object can be used as an
lvalue and potentially modified.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:208
+ std::string ExtractedVarDecl =
+ Type + " " + VarName.str() + " = " + ExtractionCode.str() + "; ";
return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
----------------
dgoldman wrote:
> sammccall wrote:
> > Type + Varname isn't correct for types in general, particularly those that
> > use declarator syntax.
> > printType with varname as the placeholder should do what you want.
> >
> > Can you add a testcase where the variable has function pointer type?
> I went ahead and added this but I couldn't think of a way to get a result
> like this without enabling this for MemberExprs, so I went ahead and
> re-enabled for them since I think it could be useful for them. LMK if I
> should revert that change. I guess if we don't want to support MemberExprs I
> should disable the ObjC property support since it probably makes to match the
> C++ equivalent?
Try:
```
int (*foo(char))(int);
void bar() {
(void)foo('c');
}
```
extracting foo('c') to subexpr should yield `int (*placeholder)(int) =
foo('c');`
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:54
const clang::Expr *Expr;
+ llvm::Optional<QualType> ExprType;
const SelectionTree::Node *ExprNode;
----------------
just QualType, it's internally nullable already
(some value in being explicit, but the code below handling two different null
values is more confusing imo)
And this is more clearly VarType - it's the type you intend for the variable,
and it's not necessarily the type of the extracted expression (see
pseudo-object stuff)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:95
Extractable = true;
+ ExprType = Expr->getType();
+ // No need to fix up the type if we're just going to use "auto" later on.
----------------
sorry, please do pull this back out into a function
(my question was just can this be a function rather than a method, it seemed
self-contained)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:97
+ // No need to fix up the type if we're just going to use "auto" later on.
+ if (Ctx.getLangOpts().CPlusPlus11)
+ return;
----------------
If you set ExprType to Ctx.getAutoDeductType(), I think you can avoid treating
this as a special case when printing
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:99
+ return;
+ if (ExprType->getTypePtrOrNull() &&
Expr->hasPlaceholderType(BuiltinType::PseudoObject)) {
+ if (const auto *PR = dyn_cast<ObjCPropertyRefExpr>(Expr)) {
----------------
nit: ExprType->getTypePtrOrNull() as a boolean is more directly
!ExprType->isNull()
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:374
// FIXME: really? What if it's e.g. `std::is_same<void, void>::value`?
- if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
return false;
----------------
The reason MemberExpr is banned is that we don't want to suggest extracting
`foo` if it happens to be a member.
You can leave it banned, or allow it but disallow the case where the
MemberExpr's base is a CXXThisExpr which is implicit.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:401
+ const Type *ExprType = E->getType().getTypePtrOrNull();
+ if (ExprType && ExprType->isVoidType())
+ return false;
----------------
I'd just change this to `if (!ExprType || ExprType->isVoidType())`
I don't think there's a real null-type case that does anything useful.
================
Comment at:
clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:338
+ })cpp"},
+ // We don't preserve types through typedefs.
+ {R"cpp(typedef long NSInteger;
----------------
This comment is inaccurate: if the expression has type `NSInteger`, that code
will be written.
The issue is that the type of the expression **isn't** `NSInteger`, it's just
`int`. **arithmetic** doesn't produce typedef types in the way I think you're
expecting.
If the expression was something that **was** an `NSInteger` (like a call to a
function whose declared return type is NSInteger) then that type would be
preserved.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124486/new/
https://reviews.llvm.org/D124486
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
