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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I had to revert this in rL354839  because 
one of the tests didn't pass on Windows:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/4641


Repository:
  rL LLVM

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

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

[Tom Roeder via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL354832: [ASTImporter] Add support for importing ChooseExpr 
AST nodes. (authored by tmroeder, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58292?vs=188262=188266#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
  cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
  cfe/trunk/test/ASTMerge/choose-expr/Inputs/choose.c
  cfe/trunk/test/ASTMerge/choose-expr/test.c
  cfe/trunk/unittests/AST/ASTImporterTest.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: cfe/trunk/docs/LibASTMatchersReference.html
===
--- cfe/trunk/docs/LibASTMatchersReference.html
+++ cfe/trunk/docs/LibASTMatchersReference.html
@@ -788,6 +788,11 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtchooseExprMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ChooseExpr.html;>ChooseExpr...
+Matches GNU __builtin_choose_expr.
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtcompoundLiteralExprMatcherhttps://clang.llvm.org/doxygen/classclang_1_1CompoundLiteralExpr.html;>CompoundLiteralExpr...
 Matches compound (i.e. non-scalar) literals
 
Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -2158,6 +2158,10 @@
 extern const internal::VariadicDynCastAllOfMatcher
 cxxNullPtrLiteralExpr;
 
+/// Matches GNU __builtin_choose_expr.
+extern const internal::VariadicDynCastAllOfMatcher
+chooseExpr;
+
 /// Matches GNU __null expression.
 extern const internal::VariadicDynCastAllOfMatcher
 gnuNullExpr;
Index: cfe/trunk/test/ASTMerge/choose-expr/Inputs/choose.c
===
--- cfe/trunk/test/ASTMerge/choose-expr/Inputs/choose.c
+++ cfe/trunk/test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: cfe/trunk/test/ASTMerge/choose-expr/test.c
===
--- cfe/trunk/test/ASTMerge/choose-expr/test.c
+++ cfe/trunk/test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -552,6 +552,7 @@
 // Importing expressions
 ExpectedStmt VisitExpr(Expr *E);
 ExpectedStmt VisitVAArgExpr(VAArgExpr *E);
+ExpectedStmt VisitChooseExpr(ChooseExpr *E);
 ExpectedStmt VisitGNUNullExpr(GNUNullExpr *E);
 ExpectedStmt VisitPredefinedExpr(PredefinedExpr *E);
 ExpectedStmt VisitDeclRefExpr(DeclRefExpr *E);
@@ -6135,6 +6136,33 @@
   E->isMicrosoftABI());
 }
 
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());
+  if (!Imp)
+return Imp.takeError();
+
+  Expr *ToCond;
+  Expr *ToLHS;
+  Expr *ToRHS;
+  SourceLocation ToBuiltinLoc, ToRParenLoc;
+  QualType ToType;
+  std::tie(ToCond, ToLHS, ToRHS, ToBuiltinLoc, ToRParenLoc, ToType) = *Imp;
+
+  ExprValueKind VK = E->getValueKind();
+  ExprObjectKind OK = E->getObjectKind();
+
+  bool TypeDependent = ToCond->isTypeDependent();
+  bool ValueDependent = ToCond->isValueDependent();
+
+  // The value of CondIsTrue only matters if the value is not
+  // condition-dependent.
+  bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
+
+  return new (Importer.getToContext())
+  ChooseExpr(ToBuiltinLoc, ToCond, ToLHS, ToRHS, ToType, VK, OK,
+ ToRParenLoc, CondIsTrue, TypeDependent, ValueDependent);
+}
 
 ExpectedStmt ASTNodeImporter::VisitGNUNullExpr(GNUNullExpr *E) {
   ExpectedType TypeOrErr = import(E->getType());
Index: cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 188262.
tmroeder added a comment.

Updating after switching to the git monorepo model.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/test/ASTMerge/choose-expr/Inputs/choose.c
  clang/test/ASTMerge/choose-expr/test.c
  clang/unittests/AST/ASTImporterTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,34 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  // This case tests C code that is not condition-dependent and has a true
+  // condition.
+  testImport(
+"void declToImport() { (void)__builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+
+  ArgVector Args = getExtraArgs();
+  BindableMatcher Matcher =
+  functionTemplateDecl(hasDescendant(chooseExpr()));
+
+  // Don't try to match the template contents if template parsing is delayed.
+  if (llvm::find(Args, "-fdelayed-template-parsing") != Args.end()) {
+Matcher = functionTemplateDecl();
+  }
+
+  // Make sure that uses of (void)__builtin_choose_expr with dependent types in
+  // the condition are handled properly. This test triggers an assertion if the
+  // ASTImporter incorrectly tries to access isConditionTrue() when
+  // isConditionDependent() is true.
+  testImport("template void declToImport() { "
+ "(void)__builtin_choose_expr(N, 1, 0); }",
+ Lang_CXX, "", Lang_CXX, Verifier, Matcher);
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
@@ -1312,6 +1340,27 @@
   ASSERT_EQ(ToTemplated1, ToTemplated);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportChooseExpr) {
+  // This tests the import of isConditionTrue directly to make sure the importer
+  // gets it right.
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+"void declToImport() { (void)__builtin_choose_expr(1, 0, 1); }",
+Lang_C, "", Lang_C);
+
+  auto ToResults = match(chooseExpr().bind("choose"), To->getASTContext());
+  auto FromResults = match(chooseExpr().bind("choose"), From->getASTContext());
+
+  const ChooseExpr *FromChooseExpr =
+  selectFirst("choose", FromResults);
+  ASSERT_TRUE(FromChooseExpr);
+
+  const ChooseExpr *ToChooseExpr = selectFirst("choose", ToResults);
+  ASSERT_TRUE(ToChooseExpr);
+
+  EXPECT_EQ(FromChooseExpr->isConditionTrue(), ToChooseExpr->isConditionTrue());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportFunctionWithBackReferringParameter) {
   Decl *From, *To;
Index: clang/test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ clang/test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: clang/test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ clang/test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   REGISTER_MATCHER(castExpr);
   REGISTER_MATCHER(characterLiteral);
+  REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
Index: 

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 188224.
tmroeder added a comment.

Changed to use llvm::find.

Given the disagreement about the destructuring assignment, I didn't make that 
change.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,34 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  // This case tests C code that is not condition-dependent and has a true
+  // condition.
+  testImport(
+"void declToImport() { (void)__builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+
+  ArgVector Args = getExtraArgs();
+  BindableMatcher Matcher =
+  functionTemplateDecl(hasDescendant(chooseExpr()));
+
+  // Don't try to match the template contents if template parsing is delayed.
+  if (llvm::find(Args, "-fdelayed-template-parsing") != Args.end()) {
+Matcher = functionTemplateDecl();
+  }
+
+  // Make sure that uses of (void)__builtin_choose_expr with dependent types in
+  // the condition are handled properly. This test triggers an assertion if the
+  // ASTImporter incorrectly tries to access isConditionTrue() when
+  // isConditionDependent() is true.
+  testImport("template void declToImport() { "
+ "(void)__builtin_choose_expr(N, 1, 0); }",
+ Lang_CXX, "", Lang_CXX, Verifier, Matcher);
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
@@ -1312,6 +1340,27 @@
   ASSERT_EQ(ToTemplated1, ToTemplated);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportChooseExpr) {
+  // This tests the import of isConditionTrue directly to make sure the importer
+  // gets it right.
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+"void declToImport() { (void)__builtin_choose_expr(1, 0, 1); }",
+Lang_C, "", Lang_C);
+
+  auto ToResults = match(chooseExpr().bind("choose"), To->getASTContext());
+  auto FromResults = match(chooseExpr().bind("choose"), From->getASTContext());
+
+  const ChooseExpr *FromChooseExpr =
+  selectFirst("choose", FromResults);
+  ASSERT_TRUE(FromChooseExpr);
+
+  const ChooseExpr *ToChooseExpr = selectFirst("choose", ToResults);
+  ASSERT_TRUE(ToChooseExpr);
+
+  EXPECT_EQ(FromChooseExpr->isConditionTrue(), ToChooseExpr->isConditionTrue());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportFunctionWithBackReferringParameter) {
   Decl *From, *To;
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   REGISTER_MATCHER(castExpr);
   REGISTER_MATCHER(characterLiteral);
+  REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

a_sidorin wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > a_sidorin wrote:
> > > > aaron.ballman wrote:
> > > > > Please don't use `auto` here; the type isn't spelled out in the 
> > > > > initialization.
> > > > Hi Aaron,
> > > > importSeq() is a helper targeted to simplify a lot of imports done 
> > > > during AST node import and related error handling. The type of this 
> > > > `importSeq()` expression is `Expected > > > *, SourceLocation, SourceLocation, QualType>>`, so I think it's better 
> > > > to leave it as `auto`.
> > > Wow. I really dislike that we basically *have* to hide the type 
> > > information because it's just too ugly to spell. I understand why we're 
> > > using `auto` based on your explanation, but it basically means I can't 
> > > understand what this code is doing without a lot more effort.
> > > 
> > > Let's keep the `auto`, but let's please stop using type compositions that 
> > > make it considerably harder to review the code. This feel like a misuse 
> > > of `std::tuple`.
> > After staring at this for longer, I no longer think it's a misuse of 
> > `std::tuple`, just... an unfortunate side-effect of the design of 
> > `importSeq()`. I don't have a good idea for how to make this easier on 
> > reviewers and other people who aren't frequently looking at the AST 
> > importing code. :-(
> I think it is a case where the type is not even important. With C++17, we 
> would just write:
> ```auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), ...)
> if (Imp)
>   return Imp.takeError();
> auto [ToCond, ToLHS, ToRHS] = *Imp;
> ```
> The exact type is not really needed here. However, if you have any ideas on 
> how to improve this and make the type more explicit - they are always welcome.
I actually don't find the structured binding version to be any more readable 
than the wrapped-tuple version -- either way, I don't see the types. What makes 
this current code somewhat-okay to me is the `std::tie()` below; that at least 
lets me figure it out through reasoning. In the structured binding version, 
that would likely go away and I'd probably argue harder for this being an 
unfortunate design for maintainers and reviewers.


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

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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

Hi Tom,
Thanks for the fixes! The patch looks good to me now. I have only a small nit 
inline.




Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

aaron.ballman wrote:
> aaron.ballman wrote:
> > a_sidorin wrote:
> > > aaron.ballman wrote:
> > > > Please don't use `auto` here; the type isn't spelled out in the 
> > > > initialization.
> > > Hi Aaron,
> > > importSeq() is a helper targeted to simplify a lot of imports done during 
> > > AST node import and related error handling. The type of this 
> > > `importSeq()` expression is `Expected > > SourceLocation, SourceLocation, QualType>>`, so I think it's better to 
> > > leave it as `auto`.
> > Wow. I really dislike that we basically *have* to hide the type information 
> > because it's just too ugly to spell. I understand why we're using `auto` 
> > based on your explanation, but it basically means I can't understand what 
> > this code is doing without a lot more effort.
> > 
> > Let's keep the `auto`, but let's please stop using type compositions that 
> > make it considerably harder to review the code. This feel like a misuse of 
> > `std::tuple`.
> After staring at this for longer, I no longer think it's a misuse of 
> `std::tuple`, just... an unfortunate side-effect of the design of 
> `importSeq()`. I don't have a good idea for how to make this easier on 
> reviewers and other people who aren't frequently looking at the AST importing 
> code. :-(
I think it is a case where the type is not even important. With C++17, we would 
just write:
```auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(), ...)
if (Imp)
  return Imp.takeError();
auto [ToCond, ToLHS, ToRHS] = *Imp;
```
The exact type is not really needed here. However, if you have any ideas on how 
to improve this and make the type more explicit - they are always welcome.



Comment at: unittests/AST/ASTImporterTest.cpp:581
+  // Don't try to match the template contents if template parsing is delayed.
+  if (std::find(std::begin(Args), std::end(Args),
+"-fdelayed-template-parsing") != Args.end()) {

LLVm has a set of nice helpers for working with containers in range -like 
style. I'd recommend you to use llvm::find here:
`if (llvm::find(Args), "-fdelayed-template-parsing") != Args.end()) { `


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

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 187990.
tmroeder added a comment.

Fixed a minor style typo.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,35 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  // This case tests C code that is not condition-dependent and has a true
+  // condition.
+  testImport(
+"void declToImport() { (void)__builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+
+  ArgVector Args = getExtraArgs();
+  BindableMatcher Matcher =
+  functionTemplateDecl(hasDescendant(chooseExpr()));
+
+  // Don't try to match the template contents if template parsing is delayed.
+  if (std::find(std::begin(Args), std::end(Args),
+"-fdelayed-template-parsing") != Args.end()) {
+Matcher = functionTemplateDecl();
+  }
+
+  // Make sure that uses of (void)__builtin_choose_expr with dependent types in
+  // the condition are handled properly. This test triggers an assertion if the
+  // ASTImporter incorrectly tries to access isConditionTrue() when
+  // isConditionDependent() is true.
+  testImport("template void declToImport() { "
+ "(void)__builtin_choose_expr(N, 1, 0); }",
+ Lang_CXX, "", Lang_CXX, Verifier, Matcher);
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
@@ -1312,6 +1341,27 @@
   ASSERT_EQ(ToTemplated1, ToTemplated);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportChooseExpr) {
+  // This tests the import of isConditionTrue directly to make sure the importer
+  // gets it right.
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+"void declToImport() { (void)__builtin_choose_expr(1, 0, 1); }",
+Lang_C, "", Lang_C);
+
+  auto ToResults = match(chooseExpr().bind("choose"), To->getASTContext());
+  auto FromResults = match(chooseExpr().bind("choose"), From->getASTContext());
+
+  const ChooseExpr *FromChooseExpr =
+  selectFirst("choose", FromResults);
+  ASSERT_TRUE(FromChooseExpr);
+
+  const ChooseExpr *ToChooseExpr = selectFirst("choose", ToResults);
+  ASSERT_TRUE(ToChooseExpr);
+
+  EXPECT_EQ(FromChooseExpr->isConditionTrue(), ToChooseExpr->isConditionTrue());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportFunctionWithBackReferringParameter) {
   Decl *From, *To;
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   REGISTER_MATCHER(castExpr);
   REGISTER_MATCHER(characterLiteral);
+  REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ 

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 187989.
tmroeder added a comment.

Added more unit tests.

Sorry for the delay; I had to dig into the details to figure out where to put 
these tests and how to structure them. Please let me know if there are better 
ways to do this.

I don't know any way to write a matcher that checks the condition of 
__builtin_choose_expr before and after import, so I added a unit test that 
matches the expr and compares the From and To ChooseExprs directly.

I've confirmed that without the negation in the condition in VisitChooseExpr, 
the new tests cause failures and with the negation, everything is clean.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,35 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  // This case tests C code that is not condition-dependent and has a true
+  // condition.
+  testImport(
+"void declToImport() { (void)__builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+
+  ArgVector Args = getExtraArgs();
+  BindableMatcher Matcher =
+  functionTemplateDecl(hasDescendant(chooseExpr()));
+
+  // Don't try to match the template contents if template parsing is delayed.
+  if (std::find(std::begin(Args), std::end(Args),
+"-fdelayed-template-parsing") != Args.end()) {
+Matcher = functionTemplateDecl();
+  }
+
+  // Make sure that uses of (void)__builtin_choose_expr with dependent types in
+  // the condition are handled properly. This test triggers an assertion if the
+  // ASTImporter incorrectly tries to access isConditionTrue() when
+  // isConditionDependent() is true.
+  testImport("template void declToImport() { "
+ "(void)__builtin_choose_expr(N, 1, 0); }",
+ Lang_CXX, "", Lang_CXX, Verifier, Matcher);
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
@@ -1312,6 +1341,27 @@
   ASSERT_EQ(ToTemplated1, ToTemplated);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportChooseExpr) {
+  // This tests the import of isConditionTrue directly to make sure the importer
+  // gets it right.
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+"void declToImport() { (void)__builtin_choose_expr(1, 0, 1); }",
+Lang_C, "", Lang_C);
+
+  auto ToResults = match(chooseExpr().bind("choose"), To->getASTContext());
+  auto FromResults = match(chooseExpr().bind("choose"), From->getASTContext());
+
+  const ChooseExpr *FromChooseExpr =
+  selectFirst("choose", FromResults);
+  ASSERT_TRUE(FromChooseExpr);
+
+  const ChooseExpr* ToChooseExpr = selectFirst("choose", ToResults);
+  ASSERT_TRUE(ToChooseExpr);
+
+  EXPECT_EQ(FromChooseExpr->isConditionTrue(), ToChooseExpr->isConditionTrue());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportFunctionWithBackReferringParameter) {
   Decl *From, *To;
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

tmroeder wrote:
> aaron.ballman wrote:
> > tmroeder wrote:
> > > a_sidorin wrote:
> > > > aaron.ballman wrote:
> > > > > tmroeder wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > a_sidorin wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > > > > > E->isConditionTrue();`
> > > > > > > > `bool CondIsTrue = E->isConditionDependent() && 
> > > > > > > > E->isConditionTrue();`?
> > > > > > > I like that even better than my suggestion. :-)
> > > > > > Wait, this doesn't have the same truth table as my original code.
> > > > > > 
> > > > > > let `CD = E->isConditionDependent()`
> > > > > > let `CT = E->isConditionTrue()`
> > > > > > 
> > > > > > in
> > > > > > 
> > > > > > ```
> > > > > > bool CondIsTrue = false;
> > > > > > if (!CD)
> > > > > >   CondIsTrue = CT;
> > > > > > ```
> > > > > > 
> > > > > > has the table for `CondIsTrue`:
> > > > > > 
> > > > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > > > | T | T | F |
> > > > > > | T | F | F |
> > > > > > | F | T | T |
> > > > > > | F | F | F |
> > > > > > i.e., if CD is true, then CondIsTrue is always false. Otherwise, 
> > > > > > it's the value of CT.
> > > > > > 
> > > > > > The suggested line has the truth table
> > > > > > 
> > > > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > > > | T | T | T |
> > > > > > | T | F | F |
> > > > > > | F | T | F |
> > > > > > | F | F | F |
> > > > > > 
> > > > > > That is, the effect of CD is swapped.
> > > > > > 
> > > > > > Aaron's suggestion matches my original table. I based my code on 
> > > > > > include/clang/AST/Expr.h line 4032, which asserts 
> > > > > > !isConditionDependent() in the implementation of isConditionTrue.
> > > > > > 
> > > > > > I realized this after I "fixed" my comment to match the 
> > > > > > implementation change. Am I missing something? Or is the assertion 
> > > > > > in Expr.h wrong? I think this should be
> > > > > > 
> > > > > > ```
> > > > > > bool CondIsTrue = !E->isConditionDependent() && 
> > > > > > E->isConditionTrue();
> > > > > > ```
> > > > > > 
> > > > > > I've changed my code to that and reverted the comment change.
> > > > > Good catch -- I think my eyes just missed the change in logic. 
> > > > > Perhaps we should add a test case that exercises this?
> > > > Wow, that's a nice catch. Sorry for the misleading.
> > > I started to look for a way to test this, then realized that while it's 
> > > possible to test the code itself, it's impossible to make a ChooseExpr 
> > > with isConditionDependent() that returns true for real code.
> > > 
> > > TL;DR: ChooseExpr is a C-only construct, and isConditionDependent is a 
> > > C++-only condition; it's always false in C.
> > > 
> > > Details:
> > > 
> > > ChooseExpr only represents __builtin_choose_expr which is only valid in 
> > > C, not C++. That means ChooseExpr::isConditionDependent() will always be 
> > > false.
> > > 
> > > The definition is
> > > 
> > > ```
> > > bool isConditionDependent() const {
> > >   return getCond()->isTypeDependent() || getCond() ->isValueDependent();
> > > }
> > > ```
> > > 
> > > However Expr::isTypeDependent() (see Expr.h line 158) is a purely C++ 
> > > property to do with templates ([temp.dep.expr]): it is true if the type 
> > > of the expression could change from one template instantiation to another.
> > > 
> > > Similarly Expr::isValueDependent() (see Expr.h line 144) is a purely C++ 
> > > property to do with templates ([temp.dep.expr]): it is true for 
> > > value-dependent types in templates.
> > > 
> > > Both will always be false in all instantiations of ChooseExpr, due to the 
> > > language difference.
> > > 
> > > So, I think ChooseExpr can be refactored to remove isConditionDependent 
> > > and change its constructor to remove TypeDependent and ValueDependent.
> > > 
> > > If it's OK with you, I'll do that in a followup patch.
> > > ChooseExpr only represents __builtin_choose_expr which is only valid in 
> > > C, not C++. 
> > 
> > We allow it in C++, though: https://godbolt.org/z/_f1DPV
> Hmm. Is that by design or chance? GCC doesn't allow it: 
> https://godbolt.org/z/kvGCk1
> 
> Maybe it shouldn't be allowed?
Anyway, that means I can add a test; I'll look into doing that.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

tmroeder wrote:
> a_sidorin wrote:
> > aaron.ballman wrote:
> > > tmroeder wrote:
> > > > aaron.ballman wrote:
> > > > > a_sidorin wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > > > E->isConditionTrue();`
> > > > > > `bool CondIsTrue = E->isConditionDependent() && 
> > > > > > E->isConditionTrue();`?
> > > > > I like that even better than my suggestion. :-)
> > > > Wait, this doesn't have the same truth table as my original code.
> > > > 
> > > > let `CD = E->isConditionDependent()`
> > > > let `CT = E->isConditionTrue()`
> > > > 
> > > > in
> > > > 
> > > > ```
> > > > bool CondIsTrue = false;
> > > > if (!CD)
> > > >   CondIsTrue = CT;
> > > > ```
> > > > 
> > > > has the table for `CondIsTrue`:
> > > > 
> > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > | T | T | F |
> > > > | T | F | F |
> > > > | F | T | T |
> > > > | F | F | F |
> > > > i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's 
> > > > the value of CT.
> > > > 
> > > > The suggested line has the truth table
> > > > 
> > > > | `CD` | `CT` |  `CondIsTrue` |
> > > > | T | T | T |
> > > > | T | F | F |
> > > > | F | T | F |
> > > > | F | F | F |
> > > > 
> > > > That is, the effect of CD is swapped.
> > > > 
> > > > Aaron's suggestion matches my original table. I based my code on 
> > > > include/clang/AST/Expr.h line 4032, which asserts 
> > > > !isConditionDependent() in the implementation of isConditionTrue.
> > > > 
> > > > I realized this after I "fixed" my comment to match the implementation 
> > > > change. Am I missing something? Or is the assertion in Expr.h wrong? I 
> > > > think this should be
> > > > 
> > > > ```
> > > > bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> > > > ```
> > > > 
> > > > I've changed my code to that and reverted the comment change.
> > > Good catch -- I think my eyes just missed the change in logic. Perhaps we 
> > > should add a test case that exercises this?
> > Wow, that's a nice catch. Sorry for the misleading.
> I started to look for a way to test this, then realized that while it's 
> possible to test the code itself, it's impossible to make a ChooseExpr with 
> isConditionDependent() that returns true for real code.
> 
> TL;DR: ChooseExpr is a C-only construct, and isConditionDependent is a 
> C++-only condition; it's always false in C.
> 
> Details:
> 
> ChooseExpr only represents __builtin_choose_expr which is only valid in C, 
> not C++. That means ChooseExpr::isConditionDependent() will always be false.
> 
> The definition is
> 
> ```
> bool isConditionDependent() const {
>   return getCond()->isTypeDependent() || getCond() ->isValueDependent();
> }
> ```
> 
> However Expr::isTypeDependent() (see Expr.h line 158) is a purely C++ 
> property to do with templates ([temp.dep.expr]): it is true if the type of 
> the expression could change from one template instantiation to another.
> 
> Similarly Expr::isValueDependent() (see Expr.h line 144) is a purely C++ 
> property to do with templates ([temp.dep.expr]): it is true for 
> value-dependent types in templates.
> 
> Both will always be false in all instantiations of ChooseExpr, due to the 
> language difference.
> 
> So, I think ChooseExpr can be refactored to remove isConditionDependent and 
> change its constructor to remove TypeDependent and ValueDependent.
> 
> If it's OK with you, I'll do that in a followup patch.
> ChooseExpr only represents __builtin_choose_expr which is only valid in C, 
> not C++. 

We allow it in C++, though: https://godbolt.org/z/_f1DPV


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

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder marked an inline comment as done.
tmroeder added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

a_sidorin wrote:
> aaron.ballman wrote:
> > tmroeder wrote:
> > > aaron.ballman wrote:
> > > > a_sidorin wrote:
> > > > > aaron.ballman wrote:
> > > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > > E->isConditionTrue();`
> > > > > `bool CondIsTrue = E->isConditionDependent() && 
> > > > > E->isConditionTrue();`?
> > > > I like that even better than my suggestion. :-)
> > > Wait, this doesn't have the same truth table as my original code.
> > > 
> > > let `CD = E->isConditionDependent()`
> > > let `CT = E->isConditionTrue()`
> > > 
> > > in
> > > 
> > > ```
> > > bool CondIsTrue = false;
> > > if (!CD)
> > >   CondIsTrue = CT;
> > > ```
> > > 
> > > has the table for `CondIsTrue`:
> > > 
> > > | `CD` | `CT` |  `CondIsTrue` |
> > > | T | T | F |
> > > | T | F | F |
> > > | F | T | T |
> > > | F | F | F |
> > > i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's the 
> > > value of CT.
> > > 
> > > The suggested line has the truth table
> > > 
> > > | `CD` | `CT` |  `CondIsTrue` |
> > > | T | T | T |
> > > | T | F | F |
> > > | F | T | F |
> > > | F | F | F |
> > > 
> > > That is, the effect of CD is swapped.
> > > 
> > > Aaron's suggestion matches my original table. I based my code on 
> > > include/clang/AST/Expr.h line 4032, which asserts !isConditionDependent() 
> > > in the implementation of isConditionTrue.
> > > 
> > > I realized this after I "fixed" my comment to match the implementation 
> > > change. Am I missing something? Or is the assertion in Expr.h wrong? I 
> > > think this should be
> > > 
> > > ```
> > > bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> > > ```
> > > 
> > > I've changed my code to that and reverted the comment change.
> > Good catch -- I think my eyes just missed the change in logic. Perhaps we 
> > should add a test case that exercises this?
> Wow, that's a nice catch. Sorry for the misleading.
I started to look for a way to test this, then realized that while it's 
possible to test the code itself, it's impossible to make a ChooseExpr with 
isConditionDependent() that returns true for real code.

TL;DR: ChooseExpr is a C-only construct, and isConditionDependent is a C++-only 
condition; it's always false in C.

Details:

ChooseExpr only represents __builtin_choose_expr which is only valid in C, not 
C++. That means ChooseExpr::isConditionDependent() will always be false.

The definition is

```
bool isConditionDependent() const {
  return getCond()->isTypeDependent() || getCond() ->isValueDependent();
}
```

However Expr::isTypeDependent() (see Expr.h line 158) is a purely C++ property 
to do with templates ([temp.dep.expr]): it is true if the type of the 
expression could change from one template instantiation to another.

Similarly Expr::isValueDependent() (see Expr.h line 144) is a purely C++ 
property to do with templates ([temp.dep.expr]): it is true for value-dependent 
types in templates.

Both will always be false in all instantiations of ChooseExpr, due to the 
language difference.

So, I think ChooseExpr can be refactored to remove isConditionDependent and 
change its constructor to remove TypeDependent and ValueDependent.

If it's OK with you, I'll do that in a followup patch.


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

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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

aaron.ballman wrote:
> tmroeder wrote:
> > aaron.ballman wrote:
> > > a_sidorin wrote:
> > > > aaron.ballman wrote:
> > > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > > E->isConditionTrue();`
> > > > `bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
> > > I like that even better than my suggestion. :-)
> > Wait, this doesn't have the same truth table as my original code.
> > 
> > let `CD = E->isConditionDependent()`
> > let `CT = E->isConditionTrue()`
> > 
> > in
> > 
> > ```
> > bool CondIsTrue = false;
> > if (!CD)
> >   CondIsTrue = CT;
> > ```
> > 
> > has the table for `CondIsTrue`:
> > 
> > | `CD` | `CT` |  `CondIsTrue` |
> > | T | T | F |
> > | T | F | F |
> > | F | T | T |
> > | F | F | F |
> > i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's the 
> > value of CT.
> > 
> > The suggested line has the truth table
> > 
> > | `CD` | `CT` |  `CondIsTrue` |
> > | T | T | T |
> > | T | F | F |
> > | F | T | F |
> > | F | F | F |
> > 
> > That is, the effect of CD is swapped.
> > 
> > Aaron's suggestion matches my original table. I based my code on 
> > include/clang/AST/Expr.h line 4032, which asserts !isConditionDependent() 
> > in the implementation of isConditionTrue.
> > 
> > I realized this after I "fixed" my comment to match the implementation 
> > change. Am I missing something? Or is the assertion in Expr.h wrong? I 
> > think this should be
> > 
> > ```
> > bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> > ```
> > 
> > I've changed my code to that and reverted the comment change.
> Good catch -- I think my eyes just missed the change in logic. Perhaps we 
> should add a test case that exercises this?
Wow, that's a nice catch. Sorry for the misleading.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

tmroeder wrote:
> aaron.ballman wrote:
> > a_sidorin wrote:
> > > aaron.ballman wrote:
> > > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > > E->isConditionTrue();`
> > > `bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
> > I like that even better than my suggestion. :-)
> Wait, this doesn't have the same truth table as my original code.
> 
> let `CD = E->isConditionDependent()`
> let `CT = E->isConditionTrue()`
> 
> in
> 
> ```
> bool CondIsTrue = false;
> if (!CD)
>   CondIsTrue = CT;
> ```
> 
> has the table for `CondIsTrue`:
> 
> | `CD` | `CT` |  `CondIsTrue` |
> | T | T | F |
> | T | F | F |
> | F | T | T |
> | F | F | F |
> i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's the 
> value of CT.
> 
> The suggested line has the truth table
> 
> | `CD` | `CT` |  `CondIsTrue` |
> | T | T | T |
> | T | F | F |
> | F | T | F |
> | F | F | F |
> 
> That is, the effect of CD is swapped.
> 
> Aaron's suggestion matches my original table. I based my code on 
> include/clang/AST/Expr.h line 4032, which asserts !isConditionDependent() in 
> the implementation of isConditionTrue.
> 
> I realized this after I "fixed" my comment to match the implementation 
> change. Am I missing something? Or is the assertion in Expr.h wrong? I think 
> this should be
> 
> ```
> bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
> ```
> 
> I've changed my code to that and reverted the comment change.
Good catch -- I think my eyes just missed the change in logic. Perhaps we 
should add a test case that exercises this?


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

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 187827.
tmroeder marked an inline comment as done.
tmroeder added a comment.

Reverted to the original semantics of CondIsTrue


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,15 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  testImport(
+"void declToImport() { __builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   REGISTER_MATCHER(castExpr);
   REGISTER_MATCHER(characterLiteral);
+  REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -727,6 +727,7 @@
 compoundLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher
 cxxNullPtrLiteralExpr;
+const internal::VariadicDynCastAllOfMatcher chooseExpr;
 const internal::VariadicDynCastAllOfMatcher gnuNullExpr;
 const internal::VariadicDynCastAllOfMatcher atomicExpr;
 const internal::VariadicDynCastAllOfMatcher stmtExpr;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -552,6 +552,7 @@
 // Importing expressions
 ExpectedStmt VisitExpr(Expr *E);
 ExpectedStmt VisitVAArgExpr(VAArgExpr *E);
+ExpectedStmt VisitChooseExpr(ChooseExpr *E);
 ExpectedStmt VisitGNUNullExpr(GNUNullExpr *E);
 ExpectedStmt VisitPredefinedExpr(PredefinedExpr *E);
 ExpectedStmt VisitDeclRefExpr(DeclRefExpr *E);
@@ -6135,6 +6136,33 @@
   E->isMicrosoftABI());
 }
 
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());
+  if (!Imp)
+return Imp.takeError();
+
+  Expr *ToCond;
+  Expr *ToLHS;
+  Expr *ToRHS;
+  SourceLocation ToBuiltinLoc, ToRParenLoc;
+  QualType ToType;
+  std::tie(ToCond, ToLHS, ToRHS, ToBuiltinLoc, ToRParenLoc, ToType) = *Imp;
+
+  ExprValueKind VK = E->getValueKind();
+  ExprObjectKind OK = E->getObjectKind();
+
+  bool TypeDependent = ToCond->isTypeDependent();
+  bool ValueDependent = ToCond->isValueDependent();
+
+  // The value of CondIsTrue only matters if the value is not
+  // condition-dependent.
+  bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
+
+  return new (Importer.getToContext())
+  ChooseExpr(ToBuiltinLoc, 

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

aaron.ballman wrote:
> a_sidorin wrote:
> > aaron.ballman wrote:
> > > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > > E->isConditionTrue();`
> > `bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
> I like that even better than my suggestion. :-)
Wait, this doesn't have the same truth table as my original code.

let `CD = E->isConditionDependent()`
let `CT = E->isConditionTrue()`

in

```
bool CondIsTrue = false;
if (!CD)
  CondIsTrue = CT;
```

has the table for `CondIsTrue`:

| `CD` | `CT` |  `CondIsTrue` |
| T | T | F |
| T | F | F |
| F | T | T |
| F | F | F |
i.e., if CD is true, then CondIsTrue is always false. Otherwise, it's the value 
of CT.

The suggested line has the truth table

| `CD` | `CT` |  `CondIsTrue` |
| T | T | T |
| T | F | F |
| F | T | F |
| F | F | F |

That is, the effect of CD is swapped.

Aaron's suggestion matches my original table. I based my code on 
include/clang/AST/Expr.h line 4032, which asserts !isConditionDependent() in 
the implementation of isConditionTrue.

I realized this after I "fixed" my comment to match the implementation change. 
Am I missing something? Or is the assertion in Expr.h wrong? I think this 
should be

```
bool CondIsTrue = !E->isConditionDependent() && E->isConditionTrue();
```

I've changed my code to that and reverted the comment change.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


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

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 187823.
tmroeder added a comment.

Fix a mistake in the comment.

CondIsTrue only matters if the value *is* condition dependent, not *is not* 
condition dependent.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,15 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  testImport(
+"void declToImport() { __builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   REGISTER_MATCHER(castExpr);
   REGISTER_MATCHER(characterLiteral);
+  REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -727,6 +727,7 @@
 compoundLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher
 cxxNullPtrLiteralExpr;
+const internal::VariadicDynCastAllOfMatcher chooseExpr;
 const internal::VariadicDynCastAllOfMatcher gnuNullExpr;
 const internal::VariadicDynCastAllOfMatcher atomicExpr;
 const internal::VariadicDynCastAllOfMatcher stmtExpr;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -552,6 +552,7 @@
 // Importing expressions
 ExpectedStmt VisitExpr(Expr *E);
 ExpectedStmt VisitVAArgExpr(VAArgExpr *E);
+ExpectedStmt VisitChooseExpr(ChooseExpr *E);
 ExpectedStmt VisitGNUNullExpr(GNUNullExpr *E);
 ExpectedStmt VisitPredefinedExpr(PredefinedExpr *E);
 ExpectedStmt VisitDeclRefExpr(DeclRefExpr *E);
@@ -6135,6 +6136,32 @@
   E->isMicrosoftABI());
 }
 
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());
+  if (!Imp)
+return Imp.takeError();
+
+  Expr *ToCond;
+  Expr *ToLHS;
+  Expr *ToRHS;
+  SourceLocation ToBuiltinLoc, ToRParenLoc;
+  QualType ToType;
+  std::tie(ToCond, ToLHS, ToRHS, ToBuiltinLoc, ToRParenLoc, ToType) = *Imp;
+
+  ExprValueKind VK = E->getValueKind();
+  ExprObjectKind OK = E->getObjectKind();
+
+  bool TypeDependent = ToCond->isTypeDependent();
+  bool ValueDependent = ToCond->isValueDependent();
+
+  // The value of CondIsTrue only matters if the value is condition-dependent.
+  bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();
+
+  return new (Importer.getToContext())
+  

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 187821.
tmroeder marked an inline comment as done.
tmroeder added a comment.

Changed the CondIsTrue RHS as suggested.

Left the `auto` based on the conclusion of the discussion.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,15 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  testImport(
+"void declToImport() { __builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   REGISTER_MATCHER(castExpr);
   REGISTER_MATCHER(characterLiteral);
+  REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -727,6 +727,7 @@
 compoundLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher
 cxxNullPtrLiteralExpr;
+const internal::VariadicDynCastAllOfMatcher chooseExpr;
 const internal::VariadicDynCastAllOfMatcher gnuNullExpr;
 const internal::VariadicDynCastAllOfMatcher atomicExpr;
 const internal::VariadicDynCastAllOfMatcher stmtExpr;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -552,6 +552,7 @@
 // Importing expressions
 ExpectedStmt VisitExpr(Expr *E);
 ExpectedStmt VisitVAArgExpr(VAArgExpr *E);
+ExpectedStmt VisitChooseExpr(ChooseExpr *E);
 ExpectedStmt VisitGNUNullExpr(GNUNullExpr *E);
 ExpectedStmt VisitPredefinedExpr(PredefinedExpr *E);
 ExpectedStmt VisitDeclRefExpr(DeclRefExpr *E);
@@ -6135,6 +6136,33 @@
   E->isMicrosoftABI());
 }
 
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());
+  if (!Imp)
+return Imp.takeError();
+
+  Expr *ToCond;
+  Expr *ToLHS;
+  Expr *ToRHS;
+  SourceLocation ToBuiltinLoc, ToRParenLoc;
+  QualType ToType;
+  std::tie(ToCond, ToLHS, ToRHS, ToBuiltinLoc, ToRParenLoc, ToType) = *Imp;
+
+  ExprValueKind VK = E->getValueKind();
+  ExprObjectKind OK = E->getObjectKind();
+
+  bool TypeDependent = ToCond->isTypeDependent();
+  bool ValueDependent = ToCond->isValueDependent();
+
+  // The value of CondIsTrue only matters if the value is not
+  // condition-dependent.
+  bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();
+
+  return new 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

aaron.ballman wrote:
> a_sidorin wrote:
> > aaron.ballman wrote:
> > > Please don't use `auto` here; the type isn't spelled out in the 
> > > initialization.
> > Hi Aaron,
> > importSeq() is a helper targeted to simplify a lot of imports done during 
> > AST node import and related error handling. The type of this `importSeq()` 
> > expression is `Expected > SourceLocation, QualType>>`, so I think it's better to leave it as `auto`.
> Wow. I really dislike that we basically *have* to hide the type information 
> because it's just too ugly to spell. I understand why we're using `auto` 
> based on your explanation, but it basically means I can't understand what 
> this code is doing without a lot more effort.
> 
> Let's keep the `auto`, but let's please stop using type compositions that 
> make it considerably harder to review the code. This feel like a misuse of 
> `std::tuple`.
After staring at this for longer, I no longer think it's a misuse of 
`std::tuple`, just... an unfortunate side-effect of the design of 
`importSeq()`. I don't have a good idea for how to make this easier on 
reviewers and other people who aren't frequently looking at the AST importing 
code. :-(


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

a_sidorin wrote:
> aaron.ballman wrote:
> > Please don't use `auto` here; the type isn't spelled out in the 
> > initialization.
> Hi Aaron,
> importSeq() is a helper targeted to simplify a lot of imports done during AST 
> node import and related error handling. The type of this `importSeq()` 
> expression is `Expected SourceLocation, QualType>>`, so I think it's better to leave it as `auto`.
Wow. I really dislike that we basically *have* to hide the type information 
because it's just too ugly to spell. I understand why we're using `auto` based 
on your explanation, but it basically means I can't understand what this code 
is doing without a lot more effort.

Let's keep the `auto`, but let's please stop using type compositions that make 
it considerably harder to review the code. This feel like a misuse of 
`std::tuple`.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

a_sidorin wrote:
> aaron.ballman wrote:
> > `bool CondIsTrue = E->isConditionDependent() ? false : 
> > E->isConditionTrue();`
> `bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?
I like that even better than my suggestion. :-)


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

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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Tom,

Thank for the fixes, now the patch looks almost fine. I have a small nit 
inline, but I think the patch can land after this fix.




Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

aaron.ballman wrote:
> Please don't use `auto` here; the type isn't spelled out in the 
> initialization.
Hi Aaron,
importSeq() is a helper targeted to simplify a lot of imports done during AST 
node import and related error handling. The type of this `importSeq()` 
expression is `Expected>`, so I think it's better to leave it as `auto`.



Comment at: lib/AST/ASTImporter.cpp:6160
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())

aaron.ballman wrote:
> `bool CondIsTrue = E->isConditionDependent() ? false : E->isConditionTrue();`
`bool CondIsTrue = E->isConditionDependent() && E->isConditionTrue();`?


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6140
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());

Please don't use `auto` here; the type isn't spelled out in the initialization.



Comment at: lib/AST/ASTImporter.cpp:6160-6162
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())
+CondIsTrue = E->isConditionTrue();

`bool CondIsTrue = E->isConditionDependent() ? false : E->isConditionTrue();`


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

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 187393.
tmroeder marked 6 inline comments as done.
tmroeder added a comment.

Thanks for the review and the suggestions for improving the tests.

This update cleans up the tests as suggested.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,11 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,15 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  testImport(
+"void declToImport() { __builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -std=c11 -emit-pch -o %t.ast %S/Inputs/choose.c
+// RUN: %clang_cc1 -std=c11 -ast-merge %t.ast -fsyntax-only -verify %s
+// expected-no-diagnostics
+
Index: test/ASTMerge/choose-expr/Inputs/choose.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose.c
@@ -0,0 +1,2 @@
+_Static_assert(__builtin_choose_expr(1, 1, 0), "Incorrect semantics of __builtin_choose_expr");
+_Static_assert(__builtin_choose_expr(0, 0, 1), "Incorrect semantics of __builtin_choose_expr");
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -147,6 +147,7 @@
   REGISTER_MATCHER(caseStmt);
   REGISTER_MATCHER(castExpr);
   REGISTER_MATCHER(characterLiteral);
+  REGISTER_MATCHER(chooseExpr);
   REGISTER_MATCHER(classTemplateDecl);
   REGISTER_MATCHER(classTemplateSpecializationDecl);
   REGISTER_MATCHER(complexType);
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -727,6 +727,7 @@
 compoundLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher
 cxxNullPtrLiteralExpr;
+const internal::VariadicDynCastAllOfMatcher chooseExpr;
 const internal::VariadicDynCastAllOfMatcher gnuNullExpr;
 const internal::VariadicDynCastAllOfMatcher atomicExpr;
 const internal::VariadicDynCastAllOfMatcher stmtExpr;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -552,6 +552,7 @@
 // Importing expressions
 ExpectedStmt VisitExpr(Expr *E);
 ExpectedStmt VisitVAArgExpr(VAArgExpr *E);
+ExpectedStmt VisitChooseExpr(ChooseExpr *E);
 ExpectedStmt VisitGNUNullExpr(GNUNullExpr *E);
 ExpectedStmt VisitPredefinedExpr(PredefinedExpr *E);
 ExpectedStmt VisitDeclRefExpr(DeclRefExpr *E);
@@ -6135,6 +6136,35 @@
   E->isMicrosoftABI());
 }
 
+ExpectedStmt ASTNodeImporter::VisitChooseExpr(ChooseExpr *E) {
+  auto Imp = importSeq(E->getCond(), E->getLHS(), E->getRHS(),
+   E->getBuiltinLoc(), E->getRParenLoc(), E->getType());
+  if (!Imp)
+return Imp.takeError();
+
+  Expr *ToCond;
+  Expr *ToLHS;
+  Expr *ToRHS;
+  SourceLocation ToBuiltinLoc, ToRParenLoc;
+  QualType ToType;
+  std::tie(ToCond, ToLHS, ToRHS, ToBuiltinLoc, ToRParenLoc, ToType) = *Imp;
+
+  ExprValueKind VK = E->getValueKind();
+  ExprObjectKind OK = E->getObjectKind();
+
+  bool TypeDependent = ToCond->isTypeDependent();
+  bool ValueDependent = ToCond->isValueDependent();
+
+  // The value of CondIsTrue only matters if the value is not
+  // condition-dependent.
+  bool CondIsTrue = false;
+  if (!E->isConditionDependent())
+CondIsTrue = 

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

[Tom Roeder via Phabricator via cfe-commits]

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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 187113.
tmroeder added a comment.

Updated Registry.cpp, regenerated the documentation, and added direct tests for 
the matcher.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58292/new/

https://reviews.llvm.org/D58292

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  test/ASTMerge/choose-expr/Inputs/choose1.c
  test/ASTMerge/choose-expr/Inputs/choose2.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -754,6 +754,28 @@
   EXPECT_TRUE(matches("int* i = nullptr;", cxxNullPtrLiteralExpr()));
 }
 
+TEST(Matcher, ChooseExpr) {
+  EXPECT_TRUE(matchesC("void f() { (void)__builtin_choose_expr(1, 2, 3); }",
+   chooseExpr()));
+
+  // The __builtin_choose_expr construction in the following test is closer to
+  // the way that builtin is used in macros in real programs, even though it is
+  // unnecessary when written out directly in the code like this.
+  EXPECT_TRUE(matchesC(R"(
+  void f() {
+int x = -10;
+int abs_x = __builtin_choose_expr(
+  __builtin_types_compatible_p(__typeof(x), unsigned int),
+  x,
+  __builtin_choose_expr(
+__builtin_types_compatible_p(__typeof(x), int),
+x < 0 ? -x : x,
+(void)0));
+(void)abs_x;
+  }
+  )", chooseExpr(hasDescendant(chooseExpr();
+}
+
 TEST(Matcher, GNUNullExpr) {
   EXPECT_TRUE(matches("int* i = __null;", gnuNullExpr()));
 }
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,15 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  testImport(
+"void declToImport() { __builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
@@ -1082,6 +1091,38 @@
   EXPECT_EQ(To, nullptr);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  // The macro in this TuDecl violates standard rules of macros (like not
+  // using a variable more than once), but it's only used to test a realistic
+  // __builtin_choose_expr construction.
+  Decl *FromTU = getTuDecl(
+  R"(
+  #define abs_int(x) __builtin_choose_expr(\
+ __builtin_types_compatible_p(__typeof(x), unsigned int),  \
+ x,\
+ __builtin_choose_expr(\
+   __builtin_types_compatible_p(__typeof(x), int), \
+   x < 0 ? -x : x, \
+   (void)0))
+  void declToImport() {
+int x = -10;
+int abs_x = abs_int(x);
+(void)abs_x;
+  }
+  )",
+  Lang_C, "input.c");
+  auto *From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
+  ASSERT_TRUE(From);
+  auto *To = Import(From, Lang_C);
+  ASSERT_TRUE(To);
+  EXPECT_TRUE(MatchVerifier().match(
+  To, functionDecl(hasName("declToImport"),
+   hasDescendant(chooseExpr(hasDescendant(chooseExpr()));
+}
+
 const internal::VariadicDynCastAllOfMatcher
 cxxPseudoDestructorExpr;
 
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/choose1.c
+// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/choose2.c
+// RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1
Index: test/ASTMerge/choose-expr/Inputs/choose2.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose2.c
@@ -0,0 +1,12 @@
+#define abs_int(x) __builtin_choose_expr( \
+__builtin_types_compatible_p(__typeof(x), unsigned int),  \
+x,\
+__builtin_choose_expr(\
+__builtin_types_compatible_p(__typeof(x), int),   \
+x < 0 ? -x : x,   \
+(void)0))
+void declToImport() {
+  int x = -10;
+  int abs_x = abs_int(x);
+  (void)abs_x;
+}
Index: test/ASTMerge/choose-expr/Inputs/choose1.c

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder added a comment.

In D58292#1399774 , @shafik wrote:

> This looks reasonable, I will wait for @martong and/or @a_sidorin to review.
>
> FYI LLDB is the other big user of ASTImpoter so it is helpful if you can run 
> `check-lldb` especially on MacOS so you can to catch regressions before 
> committing. After committing please make sure to monitor the GreenDragon 
> build bots:


I don't currently have a macOS machine to try this on, but I'll see what I can 
do next week.

I ran check-lldb with and without my change, and I don't seem to have broken 
anything new. However, check-lldb fails in a few ways at head (at least in my 
configuration; maybe I did something wrong):

   RESULT: FAILED (0 passes, 2 failures, 0 errors, 2 skipped, 0 



 
   expected failures, 0 unexpected successes)   



 




 
    



 
  Testing Time: 59.67s  



 
    



 
  Unexpected Passing Tests (2): 



 
  lldb-Suite :: functionalities/asan/TestMemoryHistory.py   



 
  lldb-Suite :: functionalities/asan/TestReportData.py  



 




 
    



 
  Failing Tests (3):



 
  lldb-Suite :: 



 
  

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

Please be sure to update Registry.cpp to expose the new AST matcher to the 
dynamic matcher infrastructure, regenerate the AST matcher documentation (run 
clang\docs\tools\dump_ast_matchers.py), and add tests for the matcher.


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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

This looks reasonable, I will wait for @martong and/or @a_sidorin to review.

FYI LLDB is the other big user of ASTImpoter so it is helpful if you can run 
`check-lldb` especially on MacOS so you can to catch regressions before 
committing. After committing please make sure to monitor the GreenDragon build 
bots:

  http://lab.llvm.org:8080/green/view/LLDB/

Thank you!


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

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

[Tom Roeder via Phabricator via cfe-commits]

tmroeder created this revision.
Herald added a reviewer: shafik.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

This allows ASTs to be merged when they contain ChooseExpr (the GNU
__builtin_choose_expr construction). This is needed, for example, for
cross-CTU analysis of C code that makes use of __builtin_choose_expr.

The node is already supported in the AST, but it didn't have a matcher
in ASTMatchers. So, this change adds the matcher and adds support to
ASTImporter.


Repository:
  rC Clang

https://reviews.llvm.org/D58292

Files:
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  test/ASTMerge/choose-expr/Inputs/choose1.c
  test/ASTMerge/choose-expr/Inputs/choose2.c
  test/ASTMerge/choose-expr/test.c
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -563,6 +563,15 @@
   stringLiteral(hasType(asString("const char [7]"));
 }
 
+TEST_P(ImportExpr, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  testImport(
+"void declToImport() { __builtin_choose_expr(1, 2, 3); }",
+Lang_C, "", Lang_C, Verifier,
+functionDecl(hasDescendant(chooseExpr(;
+}
+
 TEST_P(ImportExpr, ImportGNUNullExpr) {
   MatchVerifier Verifier;
   testImport(
@@ -1082,6 +1091,38 @@
   EXPECT_EQ(To, nullptr);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportChooseExpr) {
+  MatchVerifier Verifier;
+
+  // The macro in this TuDecl violates standard rules of macros (like not
+  // using a variable more than once), but it's only used to test a realistic
+  // __builtin_choose_expr construction.
+  Decl *FromTU = getTuDecl(
+  R"(
+  #define abs_int(x) __builtin_choose_expr(\
+ __builtin_types_compatible_p(__typeof(x), unsigned int),  \
+ x,\
+ __builtin_choose_expr(\
+   __builtin_types_compatible_p(__typeof(x), int), \
+   x < 0 ? -x : x, \
+   (void)0))
+  void declToImport() {
+int x = -10;
+int abs_x = abs_int(x);
+(void)abs_x;
+  }
+  )",
+  Lang_C, "input.c");
+  auto *From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
+  ASSERT_TRUE(From);
+  auto *To = Import(From, Lang_C);
+  ASSERT_TRUE(To);
+  EXPECT_TRUE(MatchVerifier().match(
+  To, functionDecl(hasName("declToImport"),
+   hasDescendant(chooseExpr(hasDescendant(chooseExpr()));
+}
+
 const internal::VariadicDynCastAllOfMatcher
 cxxPseudoDestructorExpr;
 
Index: test/ASTMerge/choose-expr/test.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/test.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/choose1.c
+// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/choose2.c
+// RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1
Index: test/ASTMerge/choose-expr/Inputs/choose2.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose2.c
@@ -0,0 +1,12 @@
+#define abs_int(x) __builtin_choose_expr( \
+__builtin_types_compatible_p(__typeof(x), unsigned int),  \
+x,\
+__builtin_choose_expr(\
+__builtin_types_compatible_p(__typeof(x), int),   \
+x < 0 ? -x : x,   \
+(void)0))
+void declToImport() {
+  int x = -10;
+  int abs_x = abs_int(x);
+  (void)abs_x;
+}
Index: test/ASTMerge/choose-expr/Inputs/choose1.c
===
--- /dev/null
+++ test/ASTMerge/choose-expr/Inputs/choose1.c
@@ -0,0 +1,12 @@
+#define abs_int(x) __builtin_choose_expr( \
+__builtin_types_compatible_p(__typeof(x), unsigned int),  \
+x,\
+__builtin_choose_expr(\
+__builtin_types_compatible_p(__typeof(x), int),   \
+x < 0 ? -x : x,   \
+(void)0))
+void declToImport() {
+  int x = -10;
+  int abs_x = abs_int(x);
+  (void)abs_x;
+}
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -727,6 +727,7 @@
 compoundLiteralExpr;
 const internal::VariadicDynCastAllOfMatcher
 cxxNullPtrLiteralExpr;
+const internal::VariadicDynCastAllOfMatcher chooseExpr;
 const