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

[David Goldman via Phabricator via cfe-commits]

dgoldman marked an inline comment as done.
dgoldman added inline comments.


================
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()) {
----------------
sammccall wrote:
> 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.
Gotcha, thanks. That makes sense, but it doesn't look like that has any usages and I'm not sure if it's safe to call from this context (e.g. if we can have a non ObjC property PseudoObject expr).  I guess I could modify getObjCProperty to return nullptr in those invalid cases, WDYT?


================
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);
----------------
sammccall wrote:
> 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');`
Thanks, that worked


================
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;
----------------
sammccall wrote:
> 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.
Went with the latter, but LMK. It does seem odd to me that the same expression is allowed if it's explicit - maybe we should just allow always, even if it's implicit? Xcode allows it, FWIW.


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:338
+             })cpp"},
+      // We don't preserve types through typedefs.
+      {R"cpp(typedef long NSInteger;
----------------
sammccall wrote:
> 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.
Gotcha, thanks. Deleted the comment, should I delete this test case as well?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124486/new/

https://reviews.llvm.org/D124486