ioeric added a comment.
Implementation looks good. Just some nits.
================
Comment at: lib/Tooling/Refactoring/Extract/CaptureVariables.cpp:36
+ return true;
+ // FIXME: Capture 'self'.
+ if (!VD->isLocalVarDeclOrParm())
----------------
and `this`?
================
Comment at: lib/Tooling/Refactoring/Extract/CaptureVariables.cpp:50
+
+ // FIXME: Detemine 'const' qualifier.
+
----------------
nit: s/Detemine/Determine/
It might make more sense to put these `FIXME` in class-level comment.
================
Comment at: lib/Tooling/Refactoring/Extract/CaptureVariables.h:36
+ explicit CapturedExtractedEntity(const VarDecl *VD)
+ : Kind(CapturedVarDecl), VD(VD) {}
+
----------------
Maybe a `FIXME` here for `Kind`?
================
Comment at: lib/Tooling/Refactoring/Extract/CaptureVariables.h:38
+
+ /// Print the parameter declaration for the captured entity.
+ void printParamDecl(llvm::raw_ostream &OS, const PrintingPolicy &PP) const;
----------------
Does this include trailing commas? Maybe add an example in the comment?
Same below.
================
Comment at: lib/Tooling/Refactoring/Extract/CaptureVariables.h:43
+ /// invoking the extracted function.
+ void printUseExpr(llvm::raw_ostream &OS, const PrintingPolicy &PP) const;
+
----------------
Maybe `printFunctionCallArg`? It's unclear where this is used.
================
Comment at: lib/Tooling/Refactoring/Extract/Extract.cpp:155
+ for (const auto &Capture : llvm::enumerate(Captures)) {
+ if (Capture.index())
+ OS << ", ";
----------------
nit: `Capture.index() > 0` to be more explicit.
Repository:
rL LLVM
https://reviews.llvm.org/D39706
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
