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).
* * *
@@ -163,20 +221,22 @@ match_primary (gfc_expr **result)
...
+ m = match_conditional (&e);
+ if (m != MATCH_YES)
+ {
+ return m;
+ }
Superfluous curly braces.
* * *
--- 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 ?
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.
* * *
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.
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.
* * *
+ 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.)
* * *
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 ;-)
* * *
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.)
Tobias
