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