[PATCH] D124486: [clangd] ExtractVariable support for C and Objective-C

[Sam McCall via Phabricator via cfe-commits]

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