ioeric added inline comments.
Comment at: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp:396
+ // Calculate the name of the `NsDecl` after it is moved to new namespace.
+ std::string OldNs = NsDecl->getQualifiedNameAsString();
+ llvm::StringRef Postfix = OldNs;
mehdi_amini added inline comments.
Comment at: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp:396
+ // Calculate the name of the `NsDecl` after it is moved to new namespace.
+ std::string OldNs = NsDecl->getQualifiedNameAsString();
+ llvm::StringRef Postfix =
This revision was automatically updated to reflect the committed changes.
Closed by commit rL281918: A clang tool for changing surrouding namespaces of
class/function definitions. (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D24183?vs=71689=71848#toc
Repository:
ioeric updated this revision to Diff 71689.
ioeric marked 4 inline comments as done.
ioeric added a comment.
- Updated commenting, and fix a bug in the binary.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
omtcyfz accepted this revision.
omtcyfz added a comment.
I have no other objections aswell.
LGTM.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
The patch looks good to me now.
Comment at: change-namespace/ChangeNamespace.h:44
@@ +43,3 @@
+class ChangeNamespaceTool : ast_matchers::MatchFinder::MatchCallback {
+public:
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:448
@@ +447,3 @@
+ continue;
+const std::string = FileAndNsMoves.first;
+auto = FileToReplacements[FilePath];
ioeric wrote:
> omtcyfz wrote:
> > `StringRef` here too.
>
ioeric marked an inline comment as done.
Comment at: change-namespace/ChangeNamespace.cpp:448
@@ +447,3 @@
+ continue;
+const std::string = FileAndNsMoves.first;
+auto = FileToReplacements[FilePath];
omtcyfz wrote:
> `StringRef` here too.
If this
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:448
@@ +447,3 @@
+ continue;
+const std::string = FileAndNsMoves.first;
+auto = FileToReplacements[FilePath];
`StringRef` here too.
https://reviews.llvm.org/D24183
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:200
@@ +199,3 @@
+ while (!NsSplitted.empty()) {
+// FIXME: consider code style for comments.
+Code = ("namespace " + NsSplitted.back() + " {\n" + Code +
hokein wrote:
>
ioeric updated this revision to Diff 71605.
ioeric marked 8 inline comments as done.
ioeric added a comment.
- Addressed review comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
hokein added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:200
@@ +199,3 @@
+ while (!NsSplitted.empty()) {
+// FIXME: consider code style for comments.
+Code = ("namespace " + NsSplitted.back() + " {\n" + Code +
Doesn't
ioeric added a comment.
PIng
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:359
@@ +358,3 @@
+ End, tok::semi, *Result.SourceManager, Result.Context->getLangOpts(),
+ /*SkipTrailingWhitespaceAndNewLine=*/true);
+ if (AfterSemi.isValid())
omtcyfz
ioeric updated this revision to Diff 71126.
ioeric marked 13 inline comments as done.
ioeric added a comment.
Herald added a subscriber: mgorny.
- Minor cleanup.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:232
@@ +231,3 @@
+}
+
+// FIXME(ioeric): handle the following symbols:
handle == "ioeric" here
s/FIXME(ioeric)/FIXME
Comment at:
ioeric updated this revision to Diff 70793.
ioeric marked 16 inline comments as done.
ioeric added a comment.
- Addressed reviewer comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:85
@@ +84,3 @@
+
+SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager ,
+ const LangOptions ) {
omtcyfz wrote:
> Wouldn't it be
omtcyfz added a comment.
Also +R Alex if he has some time to take a look at the code.
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
omtcyfz added a comment.
A round of mostly stylistic comments.
Comment at: change-namespace/ChangeNamespace.cpp:85
@@ +84,3 @@
+
+SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager ,
+ const LangOptions ) {
ioeric added a comment.
Ping
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:125
@@ +124,3 @@
+// applying all existing Replaces first if there is conflict.
+void addOrMergeReplacement(const tooling::Replacement ,
+ tooling::Replacements *Replaces) {
ioeric updated this revision to Diff 70588.
ioeric marked 9 inline comments as done.
ioeric added a comment.
Herald added a subscriber: beanz.
- Addressed reviewer comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
alexshap added inline comments.
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:48
@@ +47,3 @@
+
+cl::opt OldNamespace("old_namespace", cl::desc("Old namespace."),
+ cl::cat(ChangeNamespaceCategory));
probably you need
alexshap added a subscriber: alexshap.
Comment at: change-namespace/ChangeNamespace.cpp:125
@@ +124,3 @@
+// applying all existing Replaces first if there is conflict.
+void addOrMergeReplacement(const tooling::Replacement ,
+ tooling::Replacements
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:21
@@ +20,3 @@
+inline std::string formatNamespace(llvm::StringRef NS) {
+ (void)NS.ltrim(':');
+ return NS.str();
hokein wrote:
> this statement doesn't do the intended thing (It
ioeric updated this revision to Diff 70465.
ioeric marked 10 inline comments as done.
ioeric added a comment.
- Addressed some review comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
hokein added a comment.
some initial comments.
Comment at: change-namespace/CMakeLists.txt:9
@@ +8,3 @@
+ LINK_LIBS
+ clangAST
+ clangBasic
I think `clangASTMatchers` is needed here.
Comment at: change-namespace/ChangeNamespace.cpp:21
@@
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:480
@@ +479,3 @@
+Replaces = Replaces.merge(NewReplacements);
+format::FormatStyle Style = format::getStyle("file", FilePath, "google");
+// Clean up old namespaces if there is nothing in
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:480
@@ +479,3 @@
+Replaces = Replaces.merge(NewReplacements);
+format::FormatStyle Style = format::getStyle("file", FilePath, "google");
+// Clean up old namespaces if there is nothing in
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:22
@@ +21,3 @@
+ (void)NS.ltrim(':');
+ return NS.str();
+}
Yes, it is added to please `LLVM_ATTRIBUTE_UNUSED_RESULT` of
`llvm::StringRef::ltrim`.
Comment at:
ioeric updated this revision to Diff 70308.
ioeric marked 4 inline comments as done.
ioeric added a comment.
- Addressed review comments.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
omtcyfz added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:30
@@ +29,3 @@
+ std::string Result = Namespaces.front();
+ for (auto I = Namespaces.begin() + 1, E = Namespaces.end(); I != E; ++I) {
+Result += ("::" + *I).str();
Braces
ioeric updated this revision to Diff 70173.
ioeric added a comment.
- removed unintended changes.
https://reviews.llvm.org/D24183
Files:
CMakeLists.txt
change-namespace/CMakeLists.txt
change-namespace/ChangeNamespace.cpp
change-namespace/ChangeNamespace.h
omtcyfz added a comment.
In https://reviews.llvm.org/D24183#532876, @Eugene.Zelenko wrote:
> Looks like clang-refactor idea should finally go live.
https://reviews.llvm.org/D24192 up for review.
Repository:
rL LLVM
https://reviews.llvm.org/D24183
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
Looks like clang-refactor idea should finally go live.
Repository:
rL LLVM
https://reviews.llvm.org/D24183
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
36 matches
Mail list logo
