[PATCH] D58292: Add support for importing ChooseExpr AST nodes.

Tue Feb 19 09:30:00 PST 2019

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