[PATCH] D39706: [refactor][extract] Initial implementation of variable captures

[Eric Liu via Phabricator via cfe-commits]

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