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