[PATCH] D82179: Move TestClangConfig into libClangTesting and use it in AST Matchers tests

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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