[PATCH] D55068: NFC: Simplify dumpStmt child handling
Stephen Kelly via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 30 06:18:24 PST 2018
steveire marked an inline comment as done.
steveire added inline comments.
================
Comment at: lib/AST/ASTDumper.cpp:1987
+ ConstStmtVisitor<ASTDumper>::Visit(S);
+
----------------
aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > Was there something special about calling the Visit methods directly and bailing out? Your code certainly looks reasonable, but I wonder if the output is changed because it goes through the `ConstStmtVisitor` instead of directly dispatching.
> > I don't think it could have changed. By my understanding of the `StmtVisitor`, this has the same effect.
> Yeah, I don't see how it would either, but the original code smells suspicious to me. How about adding some tests for DeclStmt and GenericSelectionExpr that demonstrates explicitly this isn't changing behavior?
>
> In fact, it seems that we have some bugs in this area (with DeclStmt) already, so understanding what's changing may point out fixes. https://godbolt.org/z/tDJ-0H
A visual inspection and reading of the visitor should show that this patch is NFC. I'd welcome a test if you can make one. The test should be in the repo before this commit anyway.
> In fact, it seems that we have some bugs in this area (with DeclStmt) already, so understanding what's changing may point out fixes. https://godbolt.org/z/tDJ-0H
That is not related to this refactoring.
Also, clang-query has different output:
http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/W_kZFl
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55068/new/
https://reviews.llvm.org/D55068
More information about the cfe-commits
mailing list