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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
