www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.internals - foreach + ref bug

reply Andrea Fontana <nospam example.com> writes:
1- Take a range with a non-ref front() method.
2- Use it with a foreach(ref element; Range) (ex: myrange.each!(x 
=> writeln(x)) since "each" use a ref foreach)

(Bug: https://issues.dlang.org/show_bug.cgi?id=11934)

What happens? This code should be rejected but instead it is 
lowered and each element's ctor is called but not dtor. That 
means, in my case, that my library never frees a lot of resources 
(and relative referenced objects).

I think that (invalid!) code should trigger some kind of 
error/warning.

I tried to put here [1] this code:

if (auto fd = sfront.isFuncDeclaration())
{
   // If front is a function, it must return a ref if used with 
foreach(ref x; ...)
   if ((p.storageClass & STC.ref_) /* && front is ref */)
   {
     fs.error("`%s.front` isn't a ref", oaggr.toChars());
     goto case Terror;
   }
}

But I can't (and nobody can, apparently) tell how to check if 
front is returning a ref.

fd.type.toChars() == "pure nothrow  nogc  property ref  safe 
int() return"

But I still can't understand how this code take "ref" out from 
fd.type :)
Someone suggests to check fd.type.nextOf but it is simply "int" 
(return type of front() function)

Any hint?

Andrea

[1] 
https://github.com/dlang/dmd/blob/master/src/dmd/statementsem.d#L1463
[2] 
https://github.com/dlang/dmd/blob/master/src/dmd/hdrgen.d#L1883
Jun 28 2018
parent reply ketmar <ketmar ketmar.no-ip.org> writes:
Andrea Fontana wrote:

it is all fairly easy, i fixed (workarounded) this bug in Aliced long time 
ago[1]. patch is againd an older dmd, but it should put you on the track.


[1] http://dpaste.com/1SC0T6G
Jun 28 2018
parent reply Andrea Fontana <nospam example.com> writes:
On Thursday, 28 June 2018 at 10:39:21 UTC, ketmar wrote:

 Andrea Fontana wrote:

 it is all fairly easy, i fixed (workarounded) this bug in 
 Aliced long time ago[1]. patch is againd an older dmd, but it 
 should put you on the track.


 [1] http://dpaste.com/1SC0T6G


Thank you! Not that obvious for me, I've just started watching at 
dmd code.
Now the question is: should we accept foreach(ref) for non-ref 
ranges?

IMHO the right thing to do is to reject that code. I think this 
is going to brake a lot of existing phobos/user code so probably 
we should go through a deprecation process to have some time to 
fix all parts. And in this period we can use the workaround above.

What to do?

Andrea
Jun 28 2018
parent ketmar <ketmar ketmar.no-ip.org> writes:
Andrea Fontana wrote:


 Thank you! Not that obvious for me, I've just started watching at dmd 
 code.
 Now the question is: should we accept foreach(ref) for non-ref ranges?

 IMHO the right thing to do is to reject that code.


i don't see a good reason to do it. the only thing it does is forces a user 
to write alot of duplicated code: basically, exactly the same boilerplate, 
but one with ref, and one without ref. which, in turn, leads either to 
string mixin abuse (and impossibility of debugging/syntax highlighting), or 
to inevitable bugs from copy/paste.

this problem has no nice solution if we'll add code-generating templates to 
the picture, but just silently dropping `ref` causes much less headache, i 
believe.
Jun 28 2018