On Wed, Dec 5, 2018 at 12:00 AM Steve Kargl
<s...@troutmask.apl.washington.edu> wrote:
>
> I intend to commit the attached patch on Saturday.
Thanks for the work. I assume the patch bootstraps and passes regression tests?
RE:
> PR fortran/88228
> * expr.c (check_null, check_elemental): Work around -fdec and
> initialization with logical operators operating on integers.
I plan to review this section of the patch later today -- though the
patch hides the segfault from the PR, I need more time to determine
whether it is correct and complete.
RE:
> PR fortran/88139
> * dump-parse-tree.c (write_proc): Alternate return.
I dissent with this patch. The introduced error is meaningless and, as
mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree
is not directly the issue. The code should be rejected in parsing. In
gcc-8.1 the invalid code is accepted (without an ICE) even without the
-fc-prototypes flag: I haven't finished building the compiler with
your changes yet to see whether that is still true afterwards, but at
least the test case doesn't try this, so I strongly suspect the patch
is incomplete to fix the PR.
RE:
> PR fortran/88205
> * io.c (gfc_match_open): STATUS must be CHARACTER type.
[...]
>@@ -2161,6 +2167,12 @@ gfc_match_open (void)
>
> if (!open->file && open->status)
> {
>+ if (open->status->ts.type != BT_CHARACTER)
>+ {
>+ gfc_error ("STATUS must be a default character type at %C");
>+ goto cleanup;
>+ }
>+
> if (open->status->expr_type == EXPR_CONSTANT
> && gfc_wide_strncasecmp (open->status->value.character.string,
> "scratch", 7) != 0)
Both resolve_tag() and is_char_type() actually already catch type
mismatches for STATUS (the latter for constant expressions). The real
problem is the following condition which checks STATUS before it has
been processed yet, since NEWUNIT is processed before STATUS. I think
the correct thing to do is actually to move the NEWUNIT/UNIT if-block
after the STATUS if-block, rather than adding a new phrasing for the
same error. Then we should see:
pr88205.f90:13:29:
open (newunit=n, status=status)
1
Error: STATUS requires a scalar-default-char-expr at (1)
RE:
> PR fortran/88328
> * io.c (resolve_tag_format): Detect zero-sized array.
[...]
>Index: gcc/fortran/io.c
>===================================================================
>--- gcc/fortran/io.c (revision 266718)
>+++ gcc/fortran/io.c (working copy)
>@@ -1636,6 +1636,12 @@ resolve_tag_format (gfc_expr *e)
> gfc_expr *r;
> gfc_char_t *dest, *src;
>
>+ if (e->value.constructor == NULL)
>+ {
>+ gfc_error ("FORMAT tag at %C cannot be a zero-sized array");
>+ return false;
>+ }
>+
> n = 0;
> c = gfc_constructor_first (e->value.constructor);
> len = c->expr->value.character.length;
[...]
>@ -3231,12 +3257,21 @@ gfc_resolve_dt (gfc_dt *dt, locus *loc)
> {
> gfc_expr *e;
> io_kind k;
>+ locus loc_tmp;
>
> /* This is set in any case. */
> gcc_assert (dt->dt_io_kind);
> k = dt->dt_io_kind->value.iokind;
>
>- RESOLVE_TAG (&tag_format, dt->format_expr);
>+ loc_tmp = gfc_current_locus;
>+ gfc_current_locus = *loc;
>+ if (!resolve_tag (&tag_format, dt->format_expr))
>+ {
>+ gfc_current_locus = loc_tmp;
>+ return false;
>+ }
>+ gfc_current_locus = loc_tmp;
>+
> RESOLVE_TAG (&tag_rec, dt->rec);
> RESOLVE_TAG (&tag_spos, dt->pos);
> RESOLVE_TAG (&tag_advance, dt->advance);
Is it really true that resolve_tag_format needs the locus at
gfc_resolve_dt::loc instead of e->where as with the other errors in
resolve_tag_format? If so, are the other errors also incorrect in
using e->where? Might it then be better to pass loc from
gfc_resolve_dt down to resolve_tag/RESOLVE_TAG and then
resolve_tag_format, instead of swapping gfc_current_locus?
RE:
> PR fortran/88048
> * resolve.c (check_data_variable): Convert gfc_internal_error to
> an gfc_error. Add a nearby missing 'return false;'
[...]
> PR fortran/88025
> * expr.c (gfc_apply_init): Remove asserts and check for valid
> ts->u.cl->length.
[...]
> PR fortran/88116
> * simplify.c: Remove internal error and return gfc_bad_expr.
These look good.
A few pedantic comments:
RE:
> PR fortran/88269
> * io.c (io_constraint): Update macro. Remove incompatible use
> of io_constraint and give explicit error.
[...]
There should be two separate references to io_constraint and
check_io_constraints:
> * io.c (io_constraint): Update macro.
> (check_io_constraints) Remove incompatible use
> of io_constraint and give explicit error.
RE:
> #define io_constraint(condition,msg,arg)\
> if (condition) \
> {\
>- gfc_error(msg,arg);\
>+ if ((arg)->lb != NULL) \
>+ gfc_error(msg,arg);\
>+ else \
>+ gfc_error(msg,&gfc_current_locus);\
> m = MATCH_ERROR;\
> }
I think you could safely follow style conventions here (Comma-Space
and Function-Space-Parenthesis):
-#define io_constraint(condition,msg,arg)\
+#define io_constraint(condition, msg, arg)\
if (condition) \
{\
- gfc_error(msg,arg);\
+ if ((arg)->lb != NULL) \
+ gfc_error (msg, arg);\
+ else \
+ gfc_error (msg, &gfc_current_locus);\
m = MATCH_ERROR;\
}
RE:
>@@ -3741,11 +3779,14 @@ if (condition) \
> if (expr && expr->ts.type != BT_CHARACTER)
> {
>
>- io_constraint (gfc_pure (NULL) && (k == M_READ || k == M_WRITE),
>- "IO UNIT in %s statement at %C must be "
>+ if (gfc_pure (NULL) && (k == M_READ || k == M_WRITE))
>+ {
>+ gfc_error ("IO UNIT in %s statement at %C must be "
> "an internal file in a PURE procedure",
> io_kind_name (k));
>-
>+ return MATCH_ERROR;
>+ }
>+
> if (k == M_READ || k == M_WRITE)
> gfc_unset_implicit_pure (NULL);
> }
Trailing whitespace on line 3789 (post-patch).
RE:
> PR fortran/87945
> * decl.c (var_element): Inquiry parameter cannot be a data object.
> (match_data_constant): Inquiry parameter can be a data
> in a data statement.
[...]
>@@ -391,6 +399,14 @@ match_data_constant (gfc_expr **result)
> }
> else if (m == MATCH_YES)
> {
>+ /* If a parameter inquiry ends up here, symtree is NULL but **result
>+ contains the right constant expression. Check here. */
>+ if ((*result)->symtree == NULL
>+ && (*result)->expr_type == EXPR_CONSTANT
>+ && ((*result)->ts.type == BT_INTEGER
>+ || (*result)->ts.type == BT_REAL))
>+ return m;
>+
> /* F2018:R845 data-stmt-constant is initial-data-target.
> A data-stmt-constant shall be ... initial-data-target if and
> only if the corresponding data-stmt-object has the POINTER
Trailing whitespace on line 406 (post-patch).
Cheers,
Fritz
