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

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/

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

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:

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

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/LibASTMatcher

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

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.ll

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

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(

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

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::V

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

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/AST

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

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 w

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

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

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

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

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

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 wrot

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

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: > > > > aaro

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

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: > > > >

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

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:

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

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->isCondi

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

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.

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

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/ http

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

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/D5

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

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(

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

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(

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

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

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

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(

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

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://review

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

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

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

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) __built

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

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:

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

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

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

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 mat

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

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

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

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 anal