[PATCH] D24243: [clang-move] A prototype tool for moving class definition to new file.

[Haojian Wu via cfe-commits]

hokein added inline comments.

================
Comment at: clang-move/ClangMove.cpp:38
@@ +37,3 @@
+                          const clang::Module * /*Imported*/) override {
+    if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc))) {
+      if (IsAngled) {
----------------
ioeric wrote:
> Might want to comment on how #include "old_header" is handled here?
Currently the old_header is also copied to new file.

================
Comment at: clang-move/ClangMove.cpp:103
@@ +102,3 @@
+                                     const clang::SourceManager *SM) {
+  // Gets the ending ";".
+  auto EndLoc = clang::Lexer::getLocForEndOfToken(D->getLocEnd(), 0, *SM,
----------------
ioeric wrote:
> Consider the code below to also include trailing comment.
>     clang::SourceLocation after_semi = clang::Lexer::findLocationAfterToken(
>         D->getLocEnd, clang::tok::semi, *SM,
>         result.Context->getLangOpts(),
>         /*SkipTrailingWhitespaceAndNewLine=*/true);
> 
But this code could not handle cases where the declaration definition has no semi at the end, such as "void f() {}"

================
Comment at: clang-move/ClangMove.cpp:125
@@ +124,3 @@
+
+  // Add moved class definition and its related declarations. All declarations
+  // in same namespace are grouped together.
----------------
ioeric wrote:
> If two declarations in the same namespace are not consecutive in the vector (i.e. there is a decl in a different ns between them), will they be put into two canonical namespace blocks?
Yes, only declarations which are consecutive in the vector and in the same namespace are put in one namespace block.

================
Comment at: clang-move/ClangMove.cpp:129
@@ +128,3 @@
+  for (const auto &MovedDecl : Decls) {
+    std::vector<std::string> DeclNamespaces = GetNamespaces(MovedDecl.Decl);
+    auto CurrentIt = CurrentNamespaces.begin();
----------------
ioeric wrote:
> Is it possible to restrict all `MovedDecl.Decl` to NamedDecl so that we can use `getQualifiedNameAsString` instead of having a second implementation of retrieving namespaces. 
> 
> Also how is anonymous namespace handled here?
> 
> 
Yeah, `getQualifiedNameAsString` looks like a simpler way, but it doesn't suitable for our usage here.

The `getQualifiedNameAsString` includes the name of the class.  For instance, a class method decl like `void A::f() {}` defined in global namespace, it retruns `A::f` which is not intended. 

It handles anonymous namespace by wrapping like `namespace { ... } // namespace`, see the test.


================
Comment at: clang-move/ClangMove.cpp:199
@@ +198,3 @@
+
+  // Match functions defined in anonymous namespace.
+  Finder->addMatcher(
----------------
ioeric wrote:
> What about static functions?
Good catch, I missed it. Added.

================
Comment at: clang-move/ClangMove.cpp:210
@@ +209,3 @@
+      this);
+}
+
----------------
ioeric wrote:
> FIXME: support out-of-line member variable initializations etc? Or is it already covered?
Add support now.

================
Comment at: clang-move/ClangMove.cpp:240
@@ +239,3 @@
+                                llvm::StringRef FileName) {
+  if (!Spec.OldHeader.empty() && FileName.endswith(Spec.OldHeader)) {
+    HeaderIncludes.push_back(IncludeLine.str());
----------------
ioeric wrote:
> No braces.
> 
> Also, I am worrying if `endswith` is a safe way to compare files. For example, what if `FileName` is relative path? 
The `FileName` is from the `PPCallbacks::InclusionDirective` APIs. From my experience, it seems always to be an absolute file path either I pass a relative file path or a absolute path to clang-move, but this is no guarantee in the document.

Using `endswith` is the most reliable way so far.



================
Comment at: clang-move/ClangMove.h:39
@@ +38,3 @@
+  struct MoveDefinitionSpec {
+    std::string Name;
+    std::string OldHeader;
----------------
ioeric wrote:
> Should the `Name` be fully qualified?
It's not required. The name can be fully/partially qualified, e.g., `::a::b::X`, `a::b::X`, `b::X`, `X`, which has the same behavior with `hasName` ast matcher.

It the given name is partially qualified, it will match all the class whose name ends with the given name.

================
Comment at: clang-move/ClangMove.h:65
@@ +64,3 @@
+
+  const MoveDefinitionSpec &Spec;
+  // The Key is file path, value is the replacements being applied to the file.
----------------
ioeric wrote:
> Is there any reason why you made this a reference?
To avoid the cost of making a copy of the structure.

================
Comment at: clang-move/ClangMove.h:83
@@ +82,3 @@
+public:
+  explicit ClangMoveAction(
+      const ClangMoveTool::MoveDefinitionSpec &spec,
----------------
Eugene.Zelenko wrote:
> Is explicit necessary?
The `explicit` keyword can prevent the copy-list-initialization like `ClangMoveAction a = {spec, file_to_replacement}`. I'm fine to remove it since it is trival.

================
Comment at: clang-move/tool/ClangMoveMain.cpp:102
@@ +101,3 @@
+    for (const auto &it : Tool.getReplacements())
+      Files.insert(it.first);
+    auto WriteToJson = [&](llvm::raw_ostream& OS) {
----------------
ioeric wrote:
> Indentation.
Indentation is correct here, clang-format doesn't complain anything about it.


https://reviews.llvm.org/D24243