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