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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
