[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I guess you're looking for design review at this point.

But the most important part of that is the documentation of the high-level structure, which is entirely missing. I can't really infer it from the code because most of the design is (presumably) not implemented at this point :-)



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:36
+
+class ExtractionTarget {
+public:
----------------
I think "range" or "region" may be a clearer name here than "target".
(In extract variable, target was used to refer to the destination of extraction)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:110
+  // FIXME: Bail out when it's partial selected. Wait for selectiontree
+  // semicolon fix.
+  if (CommonAnc->Selected == SelectionTree::Selection::Partial &&
----------------
what is the fix you're waiting for?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:155
+public:
+  enum LocType { PRETARGET, TARGET, POSTTARGET, OUTSIDECONTEXT };
+  struct Reference {
----------------
These are important domain structures, can we move them out of the RecursiveASTVisitor? (which should be mostly restricted to managing state of the traversal itself)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:156
+  enum LocType { PRETARGET, TARGET, POSTTARGET, OUTSIDECONTEXT };
+  struct Reference {
+    Decl *TheDecl = nullptr;
----------------
You've called this "reference", but I think it's actually a "referenceddecl" - you don't store one of these for each reference do you?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:189
+
+TargetAnalyzer::Reference &TargetAnalyzer::getReferenceFor(VarDecl *D) {
+  assert(D && "D shouldn't be null!");
----------------
why does D need to be a vardecl?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:190
+TargetAnalyzer::Reference &TargetAnalyzer::getReferenceFor(VarDecl *D) {
+  assert(D && "D shouldn't be null!");
+  // Don't add Decls that are outside the extraction context or in PostTarget.
----------------
please don't add messages to asserts that just echo the code. In order of preference:
 - take a reference so it clearly can't be null
 - add a message explaining at a *high level* what variant is violated
 - add the assertion with no message


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:197
+
+void TargetAnalyzer::checkIfRecursiveCall(DeclRefExpr *DRE) {
+  if (Target->isInTarget(DRE->getBeginLoc()) &&
----------------
why is this handled specially, instead of being part of the general case of "referring to a decl not visible in the target scope"?

(in particular, if the context is forward-declared elsewhere, what's the problem?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:203
+
+void TargetAnalyzer::updateReferenceFor(DeclRefExpr *DRE) {
+  VarDecl *D = dyn_cast_or_null<VarDecl>(DRE->getDecl());
----------------
DRE is just one of the cases where we want to handle names.

I think it'll be easier to handle more cases by changing this to accept a Decl* and moving the DRE->getDecl() to the caller


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:207
+    return;
+  LocType DRELocType = getLocType(DRE->getBeginLoc());
+  LocType DeclLocType = getLocType(D->getBeginLoc());
----------------
(why beginloc rather than loc, throughout?)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:211
+  // Note that DRELocType is never OUTSIDECONTEXT.
+  if (DeclLocType + 1 != DRELocType)
+    return;
----------------
this line seems very suspect, what are you trying to do and why?



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:214
+  Reference &Ref = getReferenceFor(D);
+  if (DRELocType == POSTTARGET)
+    Ref.IsReferencedInPostTarget = true;
----------------
can't we skip the indirection here and just have something like Ref.markOccurrence(DRE->getBeginLoc()), and avoid dealing with the enum here at all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526