[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