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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 marked 5 inline comments as done.
gribozavr2 added a comment.

> 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`).



================
Comment at: clang/include/clang/Testing/TestClangConfig.h:18
+
+struct TestClangConfig {
+  TestLanguage Language;
----------------
ymandel wrote:
> Maybe add a brief comment explaining this struct?
Added.


================
Comment at: clang/include/clang/Testing/TestClangConfig.h:20
+  TestLanguage Language;
+  std::string Target;
+
----------------
ymandel wrote:
> Is this sufficiently obvious to the reader or is it worth commenting on the meaning of "target"?
Added a comment.


================
Comment at: clang/include/clang/Testing/TestClangConfig.h:64
+    std::string Result;
+    llvm::raw_string_ostream OS(Result);
+    OS << "{ Language=" << Language << ", Target=" << Target << " }";
----------------
ymandel wrote:
> Add include? ("llvm/Support/raw_ostream.h")
Added.


================
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) {
----------------
ymandel wrote:
> Isn't this always true since any given value can't also be other values? Should these be `&&`?
Thanks -- that was a bug, fixed!


================
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.
----------------
ymandel wrote:
> 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)?
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.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:3473
+                            Lang_CXX14, Lang_CXX17, Lang_CXX20}) {
+    TestClangConfig config;
+    config.Language = lang;
----------------
ymandel wrote:
> 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`.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82179/new/

https://reviews.llvm.org/D82179