[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Sam McCall via Phabricator via cfe-commits]

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

Thanks, this looks great! Just a couple of nits left.
Appreciate you fixing the out-of-line `ns::f()` case too. It's easier to understand the fixed logic than the broken logic.

I guess you don't have commit access yet, I can land this for you if you like. Let me know your preferred name/email for attribution.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:765
+
+  ExtractedFunc.ForwardDeclarationSyntacticDC = ExtractedFunc.SemanticDC =
+      ExtractedFunc.SyntacticDC = ExtZone.EnclosingFunction->getDeclContext();
----------------
You're setting/resetting these in lots of different places, but no need for that:

SyntacticDC = EnclosingFunction->getLexicalDeclContext()
SemanticDC = getDeclContext();

and set ForwardDeclarationSyntacticDC in captureMethodInfo(), leave it null if this isn't a method.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:770
+
+  if (isa<CXXMethodDecl>(ExtZone.EnclosingFunction)) {
+    const auto *Method =
----------------
this does one check, and is idiomatic in LLVM:

```
if (const auto *Method = llvm::dyn_cast<...>(...))
  captureMethodInfo(...);
```


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

https://reviews.llvm.org/D122698