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
