Re: [patch, fortran] power.f90 FAIL in testsuite

[Paul Brook <paul at codesourcery dot com>]

On Monday 01 November 2004 20:22, Scott Robert Ladd wrote:
> I'm looking for commentary on this proposed patch, which fixes the
> incorrect calculation of integer powers when the base is 1. The patch
> identifies power operations with an integer base of 1, and replace them
> with a constant 1. The conversion takes place in eval_intrinsic().

Why is the existing code wrong?
I can see it is sub-optimal, but don't see anything incorrect about it.

> Such a transform is logical, but I'm not certain I'm doing it in the
> most appropriate place. I picked something small, so my glaring mistakes
> could be easily identified... ;)

I don't think there is a "correct" place for it. We don't currently do any 
other transformations like this. It should probably be somehow moved into 
gfc_arith_power so that it works correctly with arrays.
See below for comments on the code itself.

> Then, of course, we get into the issue of whether or not other known
> transforms should be applied, such as for real and complex types.

If we do any, it makes sense to do it in a way that can easily be extended to 
cover similar transformations.  Similar logic can be applied to x**0, x*0, 
x+0 and x*1. AFAICS these are optimizations rather than correctness issues.

> This patch also modifies the test suite slightly, by removing the
> complex power tests from power.f90, and creating a new cpower.f90 test
> that, at this time, fails, likely due to the complex-specific PR 17603.

This is an unrelated change, and should really be submitted separately.

> The test suite is not very comprehensive, testing a few specific cases
> but not general correctness. 

I don't think the gcc testsuite aims to be a complete correctness testsuite. 
My optinion is that the gfortran testsuite should generally contain things 
that have previously broken, spot checks for correct operation of features, 
and anything else the author thinks is trying 
For that you need something like Plum Hall, the C++ abi testsuite, or NIST. I 
agree that a comprehensive free testsuite would be a good thing to have, but 
don't neccessarily think it should be part of gcc.

Having said that the gfortran testsuite is seriously lacking in some areas. We 
still have some things that are not tested at all.

> Combing integer, real, and complex tests 
> can cloud results. I'm willing to propose (as an example) test case
> examples for ** operator tests, providing better coverage and granularity.

Ok, I guess.  You should also update the comments in teh testcases teh reflect 
the new split.

+  /* Simplify power operation to constant if first op1 is 1 */
+  if (operator == INTRINSIC_POWER && op1->ts.type == BT_INTEGER)
+    {
+      if (mpz_cmp_si (op1->value.integer,1) == 0)

This is wrong, op1 could be an array.
Why to sepearate if blocks instead of a single condition?

+        {
+          result = gfc_get_expr ();
+          *result = *op1;
+          gfc_free_expr (op1);
+          gfc_free_expr (op2);
+          return result;
+        }

This shoud be
      {
 gfc_free_expr (op2);
 return op1;
      }
Your code will incorrectly free the contents of *result.

Paul