This is an archive of the discontinued LLVM Phabricator instance.

Move TestClangConfig into libClangTesting and use it in AST Matchers tests
ClosedPublic

Authored by gribozavr on Jun 19 2020, 6:35 AM.

Details

Summary

Previously, AST Matchers tests were using a custom way to run a test
with a specific C++ standard version. I'm migrating them to a shared
infrastructure to specify a Clang target from libClangTesting. I'm also
changing tests for AST Matchers to run in multiple language standards
versions, and under multiple triples that have different behavior with
regards to templates.

To keep the size of the patch manageable, in this patch I'm only
migrating one file to get the process started and get feedback on this
approach.

One caveat is that increasing the number of test configuration does
significantly increase the runtime of AST Matchers tests. On my machine,
the test runtime increases from 2.0 to 6.0s. I think it is worth the
improved test coverage.

Diff Detail

Event Timeline

gribozavr created this revision.Jun 19 2020, 6:35 AM
Herald added a project: Restricted Project. · View Herald Transcript
gribozavr2 edited the summary of this revision. (Show Details)Jun 19 2020, 6:40 AM
ymandel accepted this revision.Jun 19 2020, 11:31 AM
ymandel marked an inline comment as done.

Only nits. Really nice work. I much prefer your new system, having wrestled with config and multi-language testing in the existing framework. However, is this worth an RFC to the list?

clang/include/clang/Testing/TestClangConfig.h
19

Maybe add a brief comment explaining this struct?

21

Is this sufficiently obvious to the reader or is it worth commenting on the meaning of "target"?

65

Add include? ("llvm/Support/raw_ostream.h")

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
652

Nice!

818

Isn't this always true since any given value can't also be other values? Should these be &&?

1025

Maybe add comment explaining this restriction? I take it this was some feature support by gcc for C++03, which is also supported by clang for compatibility (at that language version)?

clang/unittests/Tooling/Syntax/TreeTest.cpp
3824

nit: maybe just

all_configs.push_back({lang, "x86_64-pc-linux-gnu"});

// Windows target is interesting to test because it enables
// `-fdelayed-template-parsing`.
all_configs.push_back({lang, "x86_64-pc-win32-msvc"});

Or, if you'd like a bit nicer, add a constructor to the struct and use emplace_back.

This revision is now accepted and ready to land.Jun 19 2020, 11:31 AM
gribozavr updated this revision to Diff 274008.Jun 29 2020, 1:51 AM

Addressed code review comments.

gribozavr2 marked 5 inline comments as done.Jun 29 2020, 1:51 AM
gribozavr2 added a subscriber: gribozavr2.

However, is this worth an RFC to the list?

I don't think so. I'm consolidating existing testing infrastructure that was already providing this functionality, and using regular parameterized tests (TEST_P).

clang/include/clang/Testing/TestClangConfig.h
19

Added.

21

Added a comment.

65

Added.

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
818

Thanks -- that was a bug, fixed!

1025

I don't understand it enough to write a comment (I could try to, but I think it is not needed for this patch). This limitation was already present (without a comment) in the original code that I'm refactoring.

Clang accepts this code in C++03 mode, however I took a quick peek at the generated AST, and found that it is different from the AST in, for example, C++11. For example, the CXXConstructExpr required by the matcher only appears in C++03, not in C++11. I think the differences in the AST are the reason for the test not working.

It could be the case that the absence of CXXConstructExpr in non-C++03 is a bug in Clang, but I don't know this area well enough to say right away without an in-depth study.

clang/unittests/Tooling/Syntax/TreeTest.cpp
3824

I didn't want to rely on brace initializers or constructors because they are less readable, and TestClangConfig has the potential to grow quite a few member variables in future.

This revision was automatically updated to reflect the committed changes.