This is an archive of the discontinued LLVM Phabricator instance.

Set traversal explicitly where needed in tests
ClosedPublic

Authored by steveire on Jan 10 2020, 11:49 AM.

Diff Detail

Event Timeline

steveire created this revision.Jan 10 2020, 11:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript

I was looking at the changes to ASTImporterTest.cpp and it not obvious to me how you determined where it was needed.

I was looking at the changes to ASTImporterTest.cpp and it not obvious to me how you determined where it was needed.

Some of the changes were reasonably obvious to me because they involved matching implicit nodes that require the as-is traversal. However, other changes did seem to come from out of nowhere (I commented on one such), and hopefully @steveire will explain.

clang/unittests/Tooling/StencilTest.cpp
63–64

Was this change made because you didn't want to accept the traversal mode as a parameter to matchStmt and force each caller to decide which mode they use? Or is there some other reason why this change was needed?

steveire marked an inline comment as done.May 17 2020, 3:50 AM
steveire added inline comments.
clang/unittests/Tooling/StencilTest.cpp
63–64

Yes, the test currently expects AsIs. If someone wanted to change that in the future, that is an issue for future work.

aaron.ballman added inline comments.May 17 2020, 8:12 AM
clang/unittests/Tooling/StencilTest.cpp
63–64

You say "the test" as though there's only one. There are numerous tests which use matchStmt. Do *all* of the tests require AsIs? (I think that's an example of what's causing some of the confusion @shafik and I had and I'm trying to ensure his question gets answered and that I'm fully understanding your changes.) From spot-checking, it looks like there are tests using matchStmt that don't need AsIs traversal and that this sort of cleanup work could happen in the future, but for now you're going with the easiest approach instead of expanding the test coverage for ignoring implicit nodes; is that correct?

steveire marked an inline comment as done.May 18 2020, 12:54 PM
steveire added inline comments.
clang/unittests/Tooling/StencilTest.cpp
63–64

I said "the" test because there is one StencilTest class inside one StencilTest.cpp. Perhaps not a word you would use, but that's not something to get stuck on hopefully.

Yes, it is possible that someone could make matchStmt take an explicit TraversalKind, but I didn't see the need to do that in this patch. This patch maintains the status quo as much as possible while also making it possible to change the default.

aaron.ballman accepted this revision.May 19 2020, 5:27 AM

LGTM!

clang/unittests/Tooling/StencilTest.cpp
63–64

I said "the" test because there is one StencilTest class inside one StencilTest.cpp. Perhaps not a word you would use, but that's not something to get stuck on hopefully.

Nope, just making sure I understood.

Yes, it is possible that someone could make matchStmt take an explicit TraversalKind, but I didn't see the need to do that in this patch.

Good to know! FWIW, it would be reasonable to do some of that work in this patch, but I don't insist. As with one of the other patches in the set, making the changes to handle either traversal kind as appropriate for the situation improves the test coverage for your changes and it helps demonstrate why this default is more simple (as opposed to making everything look more complex because the patch only adds code, doesn't remove any).

This patch maintains the status quo as much as possible while also making it possible to change the default.

That's good enough for me, thanks.

This revision is now accepted and ready to land.May 19 2020, 5:27 AM
This revision was automatically updated to reflect the committed changes.