Hi Tobias, On Wed, Oct 29, 2025 at 8:37 PM Tobias Burnus <[email protected]> wrote: > > From 8ee6b564fca20862b9da55a5fe1e8f90f82bd317 Mon Sep 17 00:00:00 2001 > > From: Yuao Ma<[email protected]> > > Date: Sun, 26 Oct 2025 14:50:28 +0800 > > Subject: [PATCH] fortran: support .NIL. in conditional arguments > > > > TBD > > I am not sure whether some longer description makes sense or if we just > want to go with the subject line and the by-file changelog – but in any > case 'TBD' looks wrong. >
I will fill in the appropriate description and changelog. I'm fairly
certain the recent version will require further revision, and the
description and changelog currently look similar to the initial
version, so I've left a TBD.
> * * *
>
> > +++ b/gcc/fortran/trans-expr.cc
> > @@ -4375,6 +4375,17 @@ gfc_conv_conditional_expr (gfc_se *se, gfc_expr
> > *expr)
> > tree condition, true_val, false_val;
> > tree type;
> >
> > + /* Handle .NIL. */
> > + if (!expr->value.conditional.condition)
> > + {
> > + if (se->want_pointer || expr->ts.type == BT_CHARACTER)
> > + se->expr = null_pointer_node;
> > + else
> > + se->expr = integer_zero_node;
> > + se->string_length = build_int_cst (gfc_charlen_type_node, 0);
> > + return;
> > + }
>
> This code looks wrong.
>
> And, indeed, I get:
>
> Error: non-trivial conversion in ‘integer_cst’
> integer(kind=16)
> integer(kind=4)
> D.4685 = 0;
>
> That's for the following:
> ----------------
> module m
> implicit none (type, external)
> contains
> subroutine sub(x)
> integer(16), value,optional :: x
> end subroutine
> end module m
>
> use m
> logical :: c1
> c1 = .true.
> call sub((c1 ? .nil. : 1_16))
> call sub((.not. c1 ? .nil. : 1_16))
> call sub((c1 ? 1_16 : .nil.))
> end
> ----------------
>
> // Note: I test both false and true branch - too easy to screw up here ...
> // and I like to do so. Likewise for nested conditions.
>
>
> How about the following? - Note the 'build_zero_cst',
> which handles also non-integers (and honors the type of
> integer).
>
I think this is great! It not only eliminates the need to manually
differentiate between value and reference types, but it's also
suitable for handling different data kinds. I've applied this change
along with some refactoring, extracting a few common variables and
slightly reordering the control flow.
> ======================================
> --- a/gcc/fortran/trans-expr.cc
> +++ b/gcc/fortran/trans-expr.cc
> @@ -4375,2 +4375,2 @@ gfc_conv_conditional_expr (gfc_se *se, gfc_expr *expr)
> - tree condition, true_val, false_val;
> - tree type;
> + tree condition, true_val = NULL_TREE, false_val;
> + tree type = NULL_TREE;
> @@ -4386,6 +4386,34 @@ gfc_conv_conditional_expr (gfc_se *se, gfc_expr *expr)
> - true_se.want_pointer = se->want_pointer;
> - gfc_conv_expr (&true_se, expr->value.conditional.true_expr);
> - true_val = true_se.expr;
> - false_se.want_pointer = se->want_pointer;
> - gfc_conv_expr (&false_se, expr->value.conditional.false_expr);
> - false_val = false_se.expr;
> + /* Handle .NIL.; note that either true_expr or false_expr is not .NIL. */
> + if (expr->value.conditional.true_expr->expr_type == EXPR_CONDITIONAL
> + && !expr->value.conditional.true_expr->value.conditional.condition)
> + ; /* defer to after false_expr. */
> + else
> + {
> + true_se.want_pointer = se->want_pointer;
> + gfc_conv_expr (&true_se, expr->value.conditional.true_expr);
> + true_val = true_se.expr;
> + type = TREE_TYPE (true_val);
> + }
> +
> + if (expr->value.conditional.false_expr->expr_type == EXPR_CONDITIONAL
> + && !expr->value.conditional.false_expr->value.conditional.condition)
> + {
> + false_val = build_zero_cst (type);
> + if (expr->ts.type == BT_CHARACTER)
> + false_se.string_length = build_int_cst (gfc_charlen_type_node, 0);
> + }
> + else
> + {
> + false_se.want_pointer = se->want_pointer;
> + gfc_conv_expr (&false_se, expr->value.conditional.false_expr);
> + false_val = false_se.expr;
> + }
> +
> + if (expr->value.conditional.true_expr->expr_type == EXPR_CONDITIONAL
> + && !expr->value.conditional.true_expr->value.conditional.condition)
> + {
> + type = TREE_TYPE (false_val);
> + true_val = build_zero_cst (type);
> + if (expr->ts.type == BT_CHARACTER)
> + true_se.string_length = build_int_cst (gfc_charlen_type_node, 0);
> + }
> @@ -4415,4 +4442,0 @@ gfc_conv_conditional_expr (gfc_se *se, gfc_expr *expr)
> - type = gfc_typenode_for_spec (&expr->ts);
> - if (se->want_pointer)
> - type = build_pointer_type (type);
> -
> ======================================
>
> Actually, I probably should not have deleted the last item – and
> it probably should be moved up before the gfc_conv_expr code block;
> The reason is that for
>
> (cond ? "ab" : "abcd")
>
> one is size 16 bit, the other size 32 bit, which is visible
> in the type.
>
> Presumably, it does not matter most of the time, but I would
> not rule out that it might in some special case.
>
I think deleting the last item is fine. I tested it locally and did
not find any issues.
> * * *
>
> > @@ -6688,16 +6699,16 @@ conv_dummy_value (gfc_se * parmse, gfc_expr * e,
> > gfc_symbol * fsym,
> > {
> > /* F2018:15.5.2.12 Argument presence and
> > restrictions on arguments not present. */
> > - if (e->expr_type == EXPR_VARIABLE
> > - && e->rank == 0
> > - && (gfc_expr_attr (e).allocatable
> > - || gfc_expr_attr (e).pointer))
> > + if ((e->expr_type == EXPR_VARIABLE && e->rank == 0
> > + && (gfc_expr_attr (e).allocatable || gfc_expr_attr (e).pointer))
> > + || e->expr_type == EXPR_CONDITIONAL)
> > {
>
> The existing code is to handle:
>
> subroutine sub(x)
> integer, optional :: x
> end
>
> allocatable(y)
> call sub(y) ! y is not allocated → regard as absent argument
>
>
> If you look at -fdump-tree-original, your current patch produces
> for '(c1 ? .nil. : 1)' odd code like:
>
> D.1 = !c1
> D.2 = !c1
>
> if (D.1)
> (void) 0
> else
> D.3 = 1;
>
> sub(D.1 ? 0 : D.2 ? 0 : 1, ...)
>
>
> I think it makes more sense to handle conditionals differently.
> How about using instead:
>
> ===============================
> @@ -6709,0 +6733,6 @@ conv_dummy_value (gfc_se * parmse, gfc_expr * e,
> gfc_symbol * fsym,
> + else if (e->expr_type == EXPR_CONDITIONAL)
> + {
> + gcc_assert (parmse && TREE_CODE (parmse->expr) == COND_EXPR);
> + tree cond = TREE_OPERAND (parmse->expr, 0);
> + vec_safe_push (optionalargs, fold_convert (boolean_type_node,
> cond));
> + }
> ===============================
>
IIUC, this approach won't work for a simple case like (.false. ? .nil.
: 1). We ultimately need to know if the value is .nil. or not, rather
than just checking the first condition. Since this seems similar to
how EXPR_VARIABLE is handled, I've just reused that logic.
> * * *
>
> Side remark: I should definitely use more testcases with
> real, character, complex, ... and not just integer.
>
> * * *
>
> > +++ b/gcc/testsuite/gfortran.dg/conditional_10.f90
> > @@ -0,0 +1,28 @@
> > +! { dg-do run }
> ...
> > + call sub((c ? "123" : .NIL.))
> > +contains
> ...
> > + subroutine sub(x)
> > + character(*), optional :: x
> > + end subroutine sub
> > +end program conditional_nil
>
> I think it would be good to have both a present
> and absent/.nil. check here. I think you could also
> check for the string LEN and the value here.
>
Done.
> * * *
> > +++ b/gcc/testsuite/gfortran.dg/conditional_12.f90
> > @@ -0,0 +1,33 @@
> > +! { dg-do run }
> ...
> > + subroutine sub_str(l, x)
> > + integer, value :: l
> > + character, value, optional :: x
> > + if (present(x)) then
> > + if (l /= LEN(x)) error stop
>
> Can we have a test which checks that the present/absent
> is correctly handled? Currently, it only checks at runtime
> the present case. The inner test is not really effective as
> 'len(x)' is statically optimized to '1'.
> (BTW: Supporting len > 1 and arrays with VALUE is a
> Fortran 2018 feature that gfortran does not yet handle.)
>
> Hence, ...
>
> > + cc = .true.
> > + call sub_str(1, (cc ? "abc" : .nil.))
> > + end
>
> I think it make sense to have a .nil. case here and
> pass the status (absent/present) as additional arg.
>
.NIL. case added.
Thanks for the review!
Yuao
0001-fortran-support-.NIL.-in-conditional-arguments.patch
Description: Binary data
