[PATCH] PR fortran/88810 -- Code clean up

[PATCH] PR fortran/88810 -- Code clean up

[Steve Kargl]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

This PR flags a section of confusing but correct code.
The attach patch rewrites the code to hopefully provide
some clarity.  It has lived my tree for around 6 months,
so has sufferred through numerous regression tests.
OK to commit?

2019-06-12  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/88810
	* dependency.c (gfc_dep_resolver): Re-arrange code to make the logic
	a bit more transparent.  Fix 2 nearby formatting issues.
 
-- 
Steve

[-- Attachment #2: dependency.diff --]
[-- Type: text/x-diff, Size: 3437 bytes --]

Index: gcc/fortran/dependency.c
===================================================================
--- gcc/fortran/dependency.c	(revision 268276)
+++ gcc/fortran/dependency.c	(working copy)
@@ -2141,7 +2141,7 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_re
 
 	  /* Index for the reverse array.  */
 	  m = -1;
-	  for (n=0; n < lref->u.ar.dimen; n++)
+	  for (n = 0; n < lref->u.ar.dimen; n++)
 	    {
 	      /* Handle dependency when either of array reference is vector
 		 subscript. There is no dependency if the vector indices
@@ -2163,7 +2163,8 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_re
 
 	      if (lref->u.ar.dimen_type[n] == DIMEN_RANGE
 		  && rref->u.ar.dimen_type[n] == DIMEN_RANGE)
-		this_dep = check_section_vs_section (&lref->u.ar, &rref->u.ar, n);
+		this_dep = check_section_vs_section (&lref->u.ar,
+						     &rref->u.ar, n);
 	      else if (lref->u.ar.dimen_type[n] == DIMEN_ELEMENT
 		       && rref->u.ar.dimen_type[n] == DIMEN_RANGE)
 		this_dep = gfc_check_element_vs_section (lref, rref, n);
@@ -2196,35 +2197,38 @@ gfc_dep_resolver (gfc_ref *lref, gfc_ref *rref, gfc_re
 	      if (rref->u.ar.dimen_type[n] == DIMEN_RANGE
 		    && lref->u.ar.dimen_type[n] == DIMEN_RANGE)
 		{
-		  /* Set reverse if backward dependence and not inhibited.  */
-		  if (reverse && reverse[m] == GFC_ENABLE_REVERSE)
-		    reverse[m] = (this_dep == GFC_DEP_BACKWARD) ?
-			         GFC_REVERSE_SET : reverse[m];
+		  if (reverse)
+		    {
+		      /* Reverse if backward dependence and not inhibited.  */
+		      if (reverse[m] == GFC_ENABLE_REVERSE
+			  && this_dep == GFC_DEP_BACKWARD)
+			reverse[m] = GFC_REVERSE_SET;
 
-		  /* Set forward if forward dependence and not inhibited.  */
-		  if (reverse && reverse[m] == GFC_ENABLE_REVERSE)
-		    reverse[m] = (this_dep == GFC_DEP_FORWARD) ?
-			         GFC_FORWARD_SET : reverse[m];
+		      /* Forward if forward dependence and not inhibited.  */
+		      if (reverse[m] == GFC_ENABLE_REVERSE
+			  && this_dep == GFC_DEP_FORWARD)
+			reverse[m] = GFC_FORWARD_SET;
 
-		  /* Flag up overlap if dependence not compatible with
-		     the overall state of the expression.  */
-		  if (reverse && reverse[m] == GFC_REVERSE_SET
-		        && this_dep == GFC_DEP_FORWARD)
-		    {
-	              reverse[m] = GFC_INHIBIT_REVERSE;
-		      this_dep = GFC_DEP_OVERLAP;
+		      /* Flag up overlap if dependence not compatible with
+			 the overall state of the expression.  */
+		      if (reverse[m] == GFC_REVERSE_SET
+			  && this_dep == GFC_DEP_FORWARD)
+			{
+			  reverse[m] = GFC_INHIBIT_REVERSE;
+			  this_dep = GFC_DEP_OVERLAP;
+			}
+		      else if (reverse[m] == GFC_FORWARD_SET
+			       && this_dep == GFC_DEP_BACKWARD)
+			{
+			  reverse[m] = GFC_INHIBIT_REVERSE;
+			  this_dep = GFC_DEP_OVERLAP;
+			}
 		    }
-		  else if (reverse && reverse[m] == GFC_FORWARD_SET
-		        && this_dep == GFC_DEP_BACKWARD)
-		    {
-	              reverse[m] = GFC_INHIBIT_REVERSE;
-		      this_dep = GFC_DEP_OVERLAP;
-		    }
 
 		  /* If no intention of reversing or reversing is explicitly
 		     inhibited, convert backward dependence to overlap.  */
-		  if ((reverse == NULL && this_dep == GFC_DEP_BACKWARD)
-		      || (reverse != NULL && reverse[m] == GFC_INHIBIT_REVERSE))
+		  if ((!reverse && this_dep == GFC_DEP_BACKWARD)
+		      || (reverse && reverse[m] == GFC_INHIBIT_REVERSE))
 		    this_dep = GFC_DEP_OVERLAP;
 		}
 

Re: [PATCH] PR fortran/88810 -- Code clean up

[Paul Richard Thomas]

Hi Steve,

As the original author of the confusing code, I want to thank you for
de-confusing it!

OK

Paul

On Wed, 12 Jun 2019 at 20:01, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
>
> This PR flags a section of confusing but correct code.
> The attach patch rewrites the code to hopefully provide
> some clarity.  It has lived my tree for around 6 months,
> so has sufferred through numerous regression tests.
> OK to commit?
>
> 2019-06-12  Steven G. Kargl  <kargl@gcc.gnu.org>
>
>         PR fortran/88810
>         * dependency.c (gfc_dep_resolver): Re-arrange code to make the logic
>         a bit more transparent.  Fix 2 nearby formatting issues.
>
> --
> Steve



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: [PATCH] PR fortran/88810 -- Code clean up

[Steve Kargl]

Committed revision 272254.

I'm sure that when you authored the code, it wasn't confusing at all.

-- 
steve


On Thu, Jun 13, 2019 at 06:14:59AM +0100, Paul Richard Thomas wrote:
> Hi Steve,
> 
> As the original author of the confusing code, I want to thank you for
> de-confusing it!
> 
> OK
> 
> Paul
> 
> On Wed, 12 Jun 2019 at 20:01, Steve Kargl
> <sgk@troutmask.apl.washington.edu> wrote:
> >
> > This PR flags a section of confusing but correct code.
> > The attach patch rewrites the code to hopefully provide
> > some clarity.  It has lived my tree for around 6 months,
> > so has sufferred through numerous regression tests.
> > OK to commit?
> >
> > 2019-06-12  Steven G. Kargl  <kargl@gcc.gnu.org>
> >
> >         PR fortran/88810
> >         * dependency.c (gfc_dep_resolver): Re-arrange code to make the logic
> >         a bit more transparent.  Fix 2 nearby formatting issues.
> >
> > --
> > Steve
> 
> 
> 
> -- 
> "If you can't explain it simply, you don't understand it well enough"
> - Albert Einstein

-- 
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow