a_sidorin requested changes to this revision.
a_sidorin added a comment.
This revision now requires changes to proceed.
Hi Tom,
The change looks reasonable but the tests need some improvements.
================
Comment at: test/ASTMerge/choose-expr/Inputs/choose1.c:1
+#define abs_int(x) __builtin_choose_expr( \
+ __builtin_types_compatible_p(__typeof(x), unsigned int), \
----------------
This test duplicates unit test. I think we can keep unit test only and remove
this one.
The other option (I like it even more) is to turn this test into constexpr
check and verify that this code _behaves_ as expected, not only that its AST
structure is fine. You can find some examples in ASTMerge tests - for expr
import, for example.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1101
+ Decl *FromTU = getTuDecl(
+ R"(
+ #define abs_int(x) __builtin_choose_expr( \
----------------
I don't think we really need such macros in unit test just to check that the
expr is imported correctly - a single valid __builtin_choose_expr is enough. It
can even be checked with a simple `testImport()` call.
================
Comment at: unittests/ASTMatchers/ASTMatchersNodeTest.cpp:765
+ EXPECT_TRUE(matchesC(R"(
+ void f() {
+ int x = -10;
----------------
Same here - I think the tests should be concise if it is possible.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58292/new/
https://reviews.llvm.org/D58292
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
