[PATCH] D27673: [clang-move] Only move used helper declarations.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the awesome suggestions on the names.  I refactored the patch, hope it is clearer now.



================
Comment at: clang-move/UsedHelperDeclFinder.cpp:30
+      Result = FD;
+      if (const auto *RD = dyn_cast<CXXRecordDecl>(FD->getParent()))
+        Result = RD;
----------------
ioeric wrote:
> hokein wrote:
> > ioeric wrote:
> > > Why do you need this? And when is a function's parent a class?
> > This is for the template method in a class.
> Wouldn't that be handled in the next iteration?
> 
> Also, if you do another `getParent` here, do you also need to update `DC`?
Yep, that would be handled in next iteration, removed it.


================
Comment at: clang-move/UsedHelperDeclFinder.cpp:103
+
+UsedHelperDeclFinder::HelperDeclsSet UsedHelperDeclFinder::getUsedHelperDecls(
+    const std::vector<const NamedDecl *> &Decls) const {
----------------
ioeric wrote:
> hokein wrote:
> > ioeric wrote:
> > > Do you assume that all nodes/decls in the graph are helpers?
> > > 
> > > What if some moved `Decl` refers to unmoved decls?
> > The node in the graph can present moved/unmoved decls and helpers.
> > 
> > > What if some moved Decl refers to unmoved decls?
> > 
> > The graph doesn't contain this information, it only contains the reference  between moved/unmoved decls and helpers. So in this case, we just move the moved Decl.
> So, IIUC, the graph can contain both non-helper decls and helper decls. In that case, why do we name it `HelperDecl` Graph? And this `getUsedHelperDecls` does not make sense either. Would be less confusing if it is just `getUsedDecls`.
Good point. Renamed to `getUsedDecls`, but I still keep the `Helper` keyword in `HelperDeclRefGraph`, because "helper" indicates we use it for helpers.


https://reviews.llvm.org/D27673