diff mbox

[Fortran] Simplify lbound

Message ID 5550ABEC.8050909@sfr.fr
State New
Headers show

Commit Message

Mikael Morin May 11, 2015, 1:17 p.m. UTC 
Le 11/05/2015 00:08, Thomas Koenig a écrit :
> Am 10.05.2015 um 22:43 schrieb H.J. Lu:
> 
>>> Here is what I have committed.
>>>
>>
>> It caused:
>>
>> /export/gnu/import/git/sources/gcc/gcc/testsuite/gfortran.dg/inline_matmul_3.f90:38:39:
>> Error: Variable 'c1' cannot appear in the expression at (1)^M
> 
> I know that error message, I got it when developing the inline
> matmul patches with the same test cases.  I had a fix for this
> error message in one of my matmul patches, but it was removed
> in the review process because it could no longer be reproduced.
> 
> So, here is the fix again.  I think it is close to obvious (since it
> fixes the problem and can obviously do no harm), but anyway:  OK for
> trunk?
> 
For what it's worth, I have looked at it further, and it seems to be
gfc_current_ns not being set to the internal namespace.
A patch like this also removes the error.

To be honest, both patches look fragile to me. Yours because it leaves
gfc_current_ns to its value, leaving the door open to other problems.
Mine, well, because it's playing with a global variable, with the
possible side-effects this could have.
However, without a better idea, I'm OK with either patch (or both).

Mikael

Comments

Thomas Koenig May 12, 2015, 6:43 a.m. UTC | #1 
Hi Mikael,


> To be honest, both patches look fragile to me. Yours because it leaves
> gfc_current_ns to its value, leaving the door open to other problems.
> Mine, well, because it's playing with a global variable, with the
> possible side-effects this could have.
> However, without a better idea, I'm OK with either patch (or both).

I have found that playing around with gfc_current_ns can be quite
dangerous and can cause regressions in unexpected places.  Specifically,
I tried wrapping the callers to create_var and insert_block in
save/restore wrappers for gfc_current_ns, and that caused quite
a few very strange regressions.

So, working on the theory that a fix that may leave unknown problems
open is better than a fix that may introduce unknown problems, and
in order to get the regression out of the way, I have committed the
patch preventing multiple resolution of an array spec.

Maybe we should open a PR for auditing the use of gfc_current_ns
in front-end optmiization.

Regards

	Thomas
Mikael Morin May 12, 2015, 10:03 a.m. UTC | #2 
Le 12/05/2015 08:43, Thomas Koenig a écrit :
> Hi Mikael,
> 
> 
>> To be honest, both patches look fragile to me. Yours because it leaves
>> gfc_current_ns to its value, leaving the door open to other problems.
>> Mine, well, because it's playing with a global variable, with the
>> possible side-effects this could have.
>> However, without a better idea, I'm OK with either patch (or both).
> 
> I have found that playing around with gfc_current_ns can be quite
> dangerous and can cause regressions in unexpected places.  Specifically,
> I tried wrapping the callers to create_var and insert_block in
> save/restore wrappers for gfc_current_ns, and that caused quite
> a few very strange regressions.
> 
> So, working on the theory that a fix that may leave unknown problems
> open is better than a fix that may introduce unknown problems, and
> in order to get the regression out of the way, I have committed the
> patch preventing multiple resolution of an array spec.
> 
thanks.
diff mbox

Patch

Index: frontend-passes.c
===================================================================
--- frontend-passes.c	(révision 223002)
+++ frontend-passes.c	(copie de travail)
@@ -581,6 +581,9 @@  insert_block ()
   else
     ns = inserted_block->ext.block.ns;

+  /* From now on, everything will happen in the inserted block.  */
+  gfc_current_ns = ns;
+
   return ns;
 }