tmroeder added inline comments.
================
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), \
----------------
a_sidorin wrote:
> 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.
I can't use constexpr (and static_assert, as in the expr tests) because those
are C++ keywords, and __builtin_choose_expr is only in C.
Instead, I used _Static_assert (from C11) to get a similar effect. Please take
a look.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1101
+ Decl *FromTU = getTuDecl(
+ R"(
+ #define abs_int(x) __builtin_choose_expr( \
----------------
a_sidorin wrote:
> 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.
OK, dropped the test, since I already have that in ImportExpr.ImportChooseExpr
in this patch.
================
Comment at: unittests/ASTMatchers/ASTMatchersNodeTest.cpp:765
+ EXPECT_TRUE(matchesC(R"(
+ void f() {
+ int x = -10;
----------------
a_sidorin wrote:
> Same here - I think the tests should be concise if it is possible.
OK, dropped this part of the test too, since the first part checks that a
simple __builtin_choose_expr matches correctly.
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
