Hi Tobias, On Fri, Sep 5, 2025 at 11:57 PM Tobias Burnus <tbur...@baylibre.com> wrote: > > Hi Yuao, > > Yuao Ma wrote: > > This time I use git format-patch. Hopefully fix the issue. > > Seem so :-) > > Do I read you patch correctly that you still want to improve it > before committing the first version? I assume so because the > changelog part of the patch is largely lacking (except for > the summary line and the mklog.sh template). >
I'll improve the existing functionality and add .nil. support in later
patches. I've updated the commit message.
(It turns out git format-patch doesn't automatically wrap lines...
I'll manually wrap the commit message to 80 characters per line in the
next update.)
> * * *
>
> > @@ -163,20 +221,22 @@ match_primary (gfc_expr **result)
> ...
> > + m = match_conditional (&e);
> > + if (m != MATCH_YES)
> > + {
> > + return m;
> > + }
>
> Superfluous curly braces.
>
Fixed.
> * * *
>
> > --- a/gcc/fortran/module.cc
> > +++ b/gcc/fortran/module.cc
> > @@ -3843,6 +3843,11 @@ mio_expr (gfc_expr **ep)
> >
> > break;
> >
> > + case EXPR_CONDITIONAL:
> > + mio_expr (&e->value.conditional.true_expr);
> > + mio_expr (&e->value.conditional.false_expr);
> > + break;
> > +
> Shouldn't you also handle e->value.conditional.condition ?
>
Fixed.
> I think a use case for modules would be something like:
>
> module m
> contains
> function f(n) result(str)
> integer, value :: n
> character(len= (n > 5 ? n : 5)) :: str
> str = ""
> str(1:5) = "abcef"
> end
> end
>
> Not extremely useful, but valid [using 'max(n,5)' is more
> readable + compatible with <= F2018]. Still, I bet that some
> real-world use case will pop up, eventually.
>
> Additional issue: This gives an internal compiler error,
> which either should be fixed (preferred) or a 'sorry'
> should be added. That's for the code path:
> gfc_match_char_spec → char_len_param_value → gfc_expr_check_typed
> → gfc_traverse_expr
>
> And the latter does not handle EXPR_CONDITIONAL.
>
Thanks for the tip! It turns out that not only does gfc_traverse_expr
fail to handle conditional expressions, but check_restricted and
gfc_check_init_expr don't either. I've added all the necessary fixes,
and the test case is now in conditional_7.f90.
(Not sure why some of these weren't caught by -Wswitch)
> * * *
>
> In matchexp.cc's match_conditional:
> > m = gfc_match_char ('?');
> ...
> > }
> <HERE>
> > m = gfc_match_expr (&true_expr);
> > if (m != MATCH_YES)
> > {
> > gfc_free_expr (condition);
> > return m;
> > }
> >
> > m = gfc_match_char (':');
> > if (m != MATCH_YES)
> > {
> ...
> > gfc_gobble_whitespace ();
> <HERE>
> > m = match_conditional (&false_expr);
>
> I would add a 'gfc_gobble_whitespace ();' at '<HERE>', i.e. before matching
> the expression.
Fixed.
> Ignoring the off-by-one issue and suboptimal diagnostic in general,
> cf. https://gcc.gnu.org/PR121750
>
> I think it makes it a bit clearer when there are many spaced or even line
> breaks as in:
>
> logical :: cond1, cond2
> cond1 = .false.
> cond2 = .false.
> print *, (cond1 ? 5 : cond2 ? &
> 6 : &
> 5.0)
> end
>
> * * *
>
> > + if (true_expr->ts.type != false_expr->ts.type)
> > + {
> > + gfc_error ("true_expr at %L and false_expr at %L in conditional "
> > + "expression must have the same declared type",
> > + &true_expr->where, &false_expr->where);
> > + return false;
> > + }
> > +
> > + if (true_expr->ts.kind != false_expr->ts.kind)
> > + {
> > + gfc_error ("true_expr at %L and false_expr at %L in conditional "
> > + "expression must have the same kind parameter",
> > + &true_expr->where, &false_expr->where);
> > + return false;
> > + }
> > +
> > + if (true_expr->rank != false_expr->rank)
> > + {
> > + gfc_error ("true_expr at %L and false_expr at %L in conditional "
> > + "expression must have the same rank",
> > + &true_expr->where, &false_expr->where);
> > + return false;
> > + }
>
> I wonder whether 'true_expr' / 'false_expr' are the best wording; Fortran
> itself just has:
>
> "R1002 conditional-expr is ( scalar-logical-expr ? expr
> [ : scalar-logical-expr ? expr ]... : expr )
> "C1004 Each expr of a conditional-expr shall have the same declared type, kind
> type parameters, and rank."
>
> and
>
> "Evaluation of a conditional-expr evaluates each scalar-logical-expr in
> order, until the value of a scalar-logical-expr is true, or there are
> no more scalar-logical-exprs. If the value of a scalar-logical-expr is
> true, its subsequent expr is chosen; otherwise, the last expr of the
> conditional-expr is chosen. The chosen expr is evaluated, and its
> value is the value of the conditional expression."
>
> I wonder whether we shouldn't just use 'expr' without true_/false_
> prefix; I am especially stumbling over the underscore but also
> "true/false expr" sounds a bit odd.
I'm open to different naming. In the GCC C frontend, the
build_conditional_expr function uses op1 and op2, while the Clang AST
uses an expression array with COND, LHS, and RHS
macros(https://github.com/llvm/llvm-project/blob/e0f00bd645bfd75a6717242cc42d63eeebb1db68/clang/include/clang/AST/Expr.h#L4390).
If I understand you correctly here, you prefer an approach similar to
Clang's?
> * * *
> > + gfc_error ("Sorry, only integer, logical, real and complex types "
> > + "are supported for conditional expressions at %L
> > currently",
>
> To me, it sounds better/clearer with currently put moved to the verb:
> 'are currently supported'. (To be corrected by native speakers.)
Fixed.
> * * *
>
> > Thanks for the thorough analysis! I haven't had a chance to look into
> > the nil part yet (I'll probably get to it tomorrow), specifically from
> > the parsing to the semantic analysis phase. Now using nil in a
> > conditional expression would result in an immediate error. Some kind
> > of coherent lol.
>
> I think for the beginning, that's okay – albeit a nicer 'sorry' would
> be better and fully working .nil. best ;-)
A nicer sorry still needs .nil. be parsed in the frontend. For now,
I'll keep it as a hard error and add the remaining functionality soon
:)
> * * *
>
>
> Just a remark regarding the valid / runtime tests: Using
> ! { dg-additional-options "-fdump-tree-original" }
> together with
> ! { dg-final { scan-tree-dump "some regex pattern" "original" } }
> ! { dg-final { scan-tree-dump-times "some regex pattern" 5 "original" } }
> ! { dg-final { scan-tree-dump-not "some regex pattern" "original" } }
>
> might be an alternative or an additional check. Here, one can check more
> explicitly for the code gen, but due to variables like 'D.1234' etc. it
> is not always easy to have a perfect test.
>
> (Besides original tree, there are many others, e.g., -fdump-tree-optimized,
> which together with -O1 or higher permits to see whether an optimization
> worked.)
>
> I am not saying that you should/have to use this feature, but I want to
> mentioned it that you are aware of it.
>
> (The front end dumping -fdump-fortran-global -fdump-fortran-optimized
> -fdump-fortran-original -fdump-parse-tree options are also nice, but they
> go to stderr and can currently not be parsed in the testsuite similar way
> to the -fdump-{tree,rtl}-* variants.)
>
Thanks for the advice! I originally tried to test dump-parse-tree for
cond-expr using dump-fortran-original, but I couldn't because of the
stderr issue :-(
> I am not completely sure how to store it, but I was wondering
> whether it should be added to operator (EXPR_OP), which is contrary
> to the others does not take any expression. As it should only be
> reachable by resolving the conditional operation, it shouldn't
> harm elsewhere but needs to be handled everywhere where
> conditional is processed. During parsing, it should be parsed after
> other options failed while during resolution/dumping, it needs to
> be handled before as, e.g., resolve_operator does not know about it
> (nor does it need to, IMHO).
I'm not sure if I'm understanding you correctly here. Are you
suggesting we treat the conditional expression as a special case for
EXPR_OP (the ternary operator)?
My initial thought, which isn't fully developed, is to store a nullptr
for the nil in the AST. However, this would require a null check every
time we use the true/false expr...
Best regards,
Yuao
0001-fortran-implement-conditional-expression-for-fortran.patch
Description: Binary data
