Re: [patch, fortran] Handling of .and. and .or. expressions

[Steve Kargl <sgk at troutmask dot apl dot washington dot edu>]

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