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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thank you!



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp:418
+  if (const auto *ME = dyn_cast<MemberExpr>(E))
+    if (const auto *TE = dyn_cast<CXXThisExpr>(ME->getBase()))
+      if (TE->isImplicit())
----------------
oops, I forgot one detail: we want ME->getBase()->IgnoreImpCasts()

(in `void nonConstMethod() { constMethod(); }` there's an ImplicitCastExpr in there...


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:326
+      // Function Pointers
+      {R"cpp(struct Handlers {
+               void (*handlerFunc)(int);
----------------
can you move the function pointer + NSInteger tests into a separate block with `ExtraArgs = {"-xc"}`?


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp:338
+             })cpp"},
+      // We don't preserve types through typedefs.
+      {R"cpp(typedef long NSInteger;
----------------
dgoldman wrote:
> 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?
Up to you, I think the test is fine.
I'd add a comment "arithmetic on typedef types yields plain integer types" or so to explain what it's about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124486