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

[Aleksei Sidorin via Phabricator via cfe-commits]

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