aaron.ballman added a comment.
In https://reviews.llvm.org/D24862#550665, @ioeric wrote:
> Sorry for the confusion. I guess I should've been clearer in the comments and
> patch summary... The changes would've been what we wanted even without the
> underlying bugs and would not be reverted after the bugs are fixed.
Ah, thank you for clarifying! Now it makes more sense to me. :-)
Actual review comments are inline.
================
Comment at: change-namespace/ChangeNamespace.cpp:279
@@ -276,2 +278,3 @@
Finder->addMatcher(
- usingDecl(hasAnyUsingShadowDecl(IsInMovedNs)).bind("using_decl"), this);
+ usingDecl(IsInMovedNs, hasAnyUsingShadowDecl(decl())).bind("using_decl"),
+ this);
----------------
Why is this change an improvement even after the underlying bug is fixed?
================
Comment at: change-namespace/ChangeNamespace.cpp:296
@@ -291,5 +295,3 @@
// definitions, so we need to exclude them in the callback handler.
- auto FuncMatcher = functionDecl(
- hasParent(namespaceDecl()),
- unless(anyOf(IsInMovedNs, hasAncestor(namespaceDecl(isAnonymous())),
- hasAncestor(cxxRecordDecl()))));
+ // NOTE: ASTMatcher crash when `FunctionDecl` is a lambda defined in
parameter
+ // list, in which case it has no parent map. Workaround by filtering out
----------------
Comment is fine, but it should be reworded once the underlying bug is fixed,
which makes me wonder if the comment is really adding value. Might make more
sense to not talk in terms of the crash, but just why you need to filter this
way.
================
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:73
@@ -71,2 +72,3 @@
int main(int argc, const char **argv) {
+ sys::PrintStackTraceOnErrorSignal(argv[0]);
tooling::CommonOptionsParser OptionsParser(argc, argv,
----------------
I don't think these changes are necessary as part of this patch.
================
Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:325
@@ +324,3 @@
+
+ // FIXME: don't add namespace qualifier when there is UsingShadowDecl.
+ std::string Expected = "namespace glob {\n"
----------------
May want to duplicate this FIXME to where we are generating the change?
================
Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:350
@@ +349,3 @@
+
+ // FIXME: don't add namespace qualifier when there is "using namespace" decl.
+ std::string Expected = "namespace glob {\n"
----------------
Same here as above.
https://reviews.llvm.org/D24862
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
