ymandel accepted this revision.
ymandel marked an inline comment as done.
ymandel added a comment.
This revision is now accepted and ready to land.
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?
================
Comment at: clang/include/clang/Testing/TestClangConfig.h:18
+
+struct TestClangConfig {
+ TestLanguage Language;
----------------
Maybe add a brief comment explaining this struct?
================
Comment at: clang/include/clang/Testing/TestClangConfig.h:20
+ TestLanguage Language;
+ std::string Target;
+
----------------
Is this sufficiently obvious to the reader or is it worth commenting on the
meaning of "target"?
================
Comment at: clang/include/clang/Testing/TestClangConfig.h:64
+ std::string Result;
+ llvm::raw_string_ostream OS(Result);
+ OS << "{ Language=" << Language << ", Target=" << Target << " }";
----------------
Add include? ("llvm/Support/raw_ostream.h")
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:652
+
+ if (!GetParam().hasDelayedTemplateParsing()) {
+ // FIXME: Fix this test to work with delayed template parsing.
----------------
Nice!
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:818
+TEST_P(ASTMatchersTest, MaterializeTemporaryExpr_MatchesTemporaryCXX11CXX14) {
+ if (GetParam().Language != Lang_CXX11 || GetParam().Language != Lang_CXX14 ||
+ GetParam().Language != Lang_CXX17) {
----------------
Isn't this always true since any given value can't also be other values? Should
these be `&&`?
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1026
+TEST_P(ASTMatchersTest, Initializers_CXX) {
+ if (GetParam().Language != Lang_CXX03) {
+ // FIXME: Make this test pass with other C++ standard versions.
----------------
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)?
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:3473
+ Lang_CXX14, Lang_CXX17, Lang_CXX20}) {
+ TestClangConfig config;
+ config.Language = lang;
----------------
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`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82179/new/
https://reviews.llvm.org/D82179
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
