ioeric added a comment.
Thanks for the comments!
================
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);
----------------
aaron.ballman wrote:
> Why is this change an improvement even after the underlying bug is fixed?
I think `IsInMovedNs` makes more sense to be a filter for using decl, and it is
also more restrictive than `hasAnyUsingShadowDecl`, which would make the
failure matching stop earlier IMO.
================
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
----------------
aaron.ballman wrote:
> 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.
Fair enough. I think I'll just get rid of the comment regarding crash and let
the unittest handle it.
================
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,
----------------
aaron.ballman wrote:
> I don't think these changes are necessary as part of this patch.
Removed.
https://reviews.llvm.org/D24862
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
