On Mon, Jun 11, 2018 at 09:22:27PM +0200, Thomas Koenig wrote:
>
> Regression-tested (which found one bug in the testsuite).
>
> OK for trunk?
>
I fine with the intent of the patch (see below).
PS: I think there may be some confusion on what IMPURE implies.
> Index: fortran/resolve.c
> ===================================================================
> --- fortran/resolve.c (Revision 261388)
> +++ fortran/resolve.c (Arbeitskopie)
> @@ -3807,7 +3807,43 @@ lookup_uop_fuzzy (const char *op, gfc_symtree *uop
> return gfc_closest_fuzzy_match (op, candidates);
> }
>
> +/* Callback finding an impure function as an operand to an .and. or
> + .or. expression. Remember the last function warned about to
> + avoid double warnings when recursing. */
>
> +static int
> +impure_function_callback (gfc_expr **e, int *walk_subtrees ATTRIBUTE_UNUSED,
> + void *data)
> +{
> + gfc_expr *f = *e;
> + const char *name;
> + static gfc_expr *last = NULL;
> + bool *found = (bool *) data;
> +
> + if (f->expr_type == EXPR_FUNCTION)
> + {
> + *found = 1;
Why not true? *found is declared to be bool.
> + if (name)
> + gfc_warning (OPT_Wsurprising, "Impure function %qs at %L "
> + "might not be evaluated", name, &f->where);
> + else
> + gfc_warning (OPT_Wsurprising, "Impure function at %L "
> + "might not be evaluated", &f->where);
I think that this can simply be "Function %qs at %L may not be evaluated"
> + /* Some people code which depends on the short-circuiting that
> + Fortran does not provide, such as
The above seems a little muddled. I suggest a shorter comment.
"Some programmers assume that Fortran supports short-circuiting in logical
expression, which can lead to surprising behavior. For example, in the
following
> +
> + if (associated(m) .and. m%t) then
m%t may be evaluated before associated(m).
> + So, warn about this idiom. However, avoid breaking
> + it on purpose. */
> +
> + if (op1->expr_type == EXPR_FUNCTION && op1->value.function.isym
> + && op1->value.function.isym->id == GFC_ISYM_ASSOCIATED)
> + {
> + gfc_expr *e = op1->value.function.actual->expr;
> + gfc_expr *en = op1->value.function.actual->next->expr;
> + if (en == NULL && gfc_check_dependency (e, op2, true))
> + {
> + gfc_warning (OPT_Wsurprising, "%qs function call at %L
> does "
> + "not guard expression at %L", "ASSOCIATED",
> + &op1->where, &op2->where);
> + dont_move = true;
> + }
> + }
> +
> + /* A bit of optimization: Transfer if (f(x) .and. flag)
> + into if (flag .and. f(x)), to save evaluation of a
s/transfer/transform
I would also put quotes around the Fortran code.
> + function. The middle end should be capable of doing
> + this with a TRUTH_AND_EXPR, but it currently does not do
> + so. See PR 85599. */
The rest looks ok, but I'm not sure if this addresses Janus'
concerns.
--
Steve
