On Monday, 17 March 2014 at 15:37:15 UTC, Andrei Alexandrescu wrote:
> On 3/17/14, 6:57 AM, Dicebot wrote:
>> Closing this seeing as Andrei has just merged it as std.internal into
>> master. I am very angry about the way it has happened.
>
> What happened now??
>
> Andrei

1)
Walter has been pushing for getting this through the review queue to the point where I needed to ask Brian to delay voting for his module and switch to proceeding with Walter's. It didn't do any harm this time as Brian got busy anyway but I am very unhappy that I even had to do it.

Now it suddenly gets cancelled and merged, internal or not (the very existence of std.internal rings a bell but it is a different story). Why bother me and push on Brian if you are just going to hurry merge it?

2)
There has been several very important concerns raised by monarch_dodra about how this specific implementation fits into D type system. He still finds absolutely horrible lines of code in that PR thread right here and now. I am absolutely ashamed of the fact that we have now non-legacy code in Phobos that breaks the immutable/const system (most recent finding).

Some of such concerns has been straight rejected with appeal to authority and those who asked have been treated as if it is their guilt. You can't both try to sell D as community project and practice such workflow.

3)
I have been asking in that PR why this proposal is even considered urgent when it looks like unexpected emergency focus put in completely wrong moment, before addressing basic issue of same domain. It wasn't only my question but also seconded by Martin Nowak. There is only one answer from Walter which does not actually answer any of those questions but continues that ridiculous "performance-performance-performance" main line.

This looks terribly like panic-driven development.

On 3/17/2014 7:33 AM, monarch_dodra wrote:
> The fix for pointers hasn't been integrated. "ScopeBuff!(int*)" still doesn't
> compile.

I had fixed that.

  import std.internal.scopebuffer;

  void main() {
    alias ScopeBuffer!(int*) T;
  }

compiles.

On Monday, 17 March 2014 at 18:37:07 UTC, Walter Bright wrote:
> On 3/17/2014 7:33 AM, monarch_dodra wrote:
>> The fix for pointers hasn't been integrated. "ScopeBuff!(int*)" still doesn't
>> compile.
>
> I had fixed that.
>
>   import std.internal.scopebuffer;
>
>   void main() {
>     alias ScopeBuffer!(int*) T;
>   }
>
> compiles.

@walter: You fixed it by casting away the const in the implementation. It didn't even cross my mind you'd fix it like that, so when I looked at the pulled code, I just saw "put(const(T)[] arr)", and figured it wasn't "fixed". My apologies for claiming your code doesn't compile.

So yes, it compiles, but the implementation is still wrong. Your implementation allows assigning an immutable(int)* into an int*.

Unless there is a performance specific reason for using the const, I don't see why we aren't removing it.

On 3/17/2014 11:10 AM, Dicebot wrote:
> 1)
> Walter has been pushing for getting this through the review queue to the point
> where I needed to ask Brian to delay voting for his module and switch to
> proceeding with Walter's. It didn't do any harm this time as Brian got busy
> anyway but I am very unhappy that I even had to do it.
>
> Now it suddenly gets cancelled and merged, internal or not (the very existence
> of std.internal rings a bell but it is a different story). Why bother me and
> push on Brian if you are just going to hurry merge it?

The ongoing threads about it made it clear that it was never going to happen as std.buffer.scopebuffer. I had assumed you were monitoring that, and I shouldn't have assumed so. I apologize.

The point of moving it to std.internal was so it would not be a documented feature of Phobos. It does take care to use successfully, and the move was in response to concerns that this would make it not in the spirit of Phobos.


> 2)
> There has been several very important concerns raised by monarch_dodra about how
> this specific implementation fits into D type system. He still finds absolutely
> horrible lines of code in that PR thread right here and now. I am absolutely
> ashamed of the fact that we have now non-legacy code in Phobos that breaks the
> immutable/const system (most recent finding).

The objective technical issues raised were all addressed, and the immutable/const one was corrected and unittests added for it before it was pulled.


> Some of such concerns has been straight rejected with appeal to authority and
> those who asked have been treated as if it is their guilt. You can't both try to
> sell D as community project and practice such workflow.

Some of the issues were subjective, where there are no clearly right or wrong answers, and a decision needs to be made at some point.


> 3)
> I have been asking in that PR why this proposal is even considered urgent when
> it looks like unexpected emergency focus put in completely wrong moment, before
> addressing basic issue of same domain. It wasn't only my question but also
> seconded by Martin Nowak. There is only one answer from Walter which does not
> actually answer any of those questions but continues that ridiculous
> "performance-performance-performance" main line.
>
> This looks terribly like panic-driven development.

ScopeBuffer has been there and commented on for about 2 months now. At last count it had over 4 comments per line of code. It is probably the most reviewed PR ever.

It is necessary to resolve a serious issue we have with Phobos that comes up constantly in public discussions about D. At some point we've got to move on and resolve this.

For reference, the two threads about it are:

https://github.com/D-Programming-Language/phobos/pull/1911

https://github.com/D-Programming-Language/druntime/pull/739

On 2014-03-17 20:07, Walter Bright wrote:

> ScopeBuffer has been there and commented on for about 2 months now.

That's because you created a pull request instead of asking for a formal review. A pull request should NOT be created until a module is accepted after a review and voting.

You're sneaking in a new module although it has not gone through a formal review. This is not how new modules should be added.

-- 
/Jacob Carlborg

On 3/17/2014 12:03 PM, monarch_dodra wrote:
> So yes, it compiles, but the implementation is still wrong. Your implementation
> allows assigning an immutable(int)* into an int*.
>
> Unless there is a performance specific reason for using the const, I don't see
> why we aren't removing it.

The reason was I was alternately calling put() with mutable char[] and immutable strings. I should look and see if there's a better way.

On 3/17/2014 12:28 PM, Jacob Carlborg wrote:
> On 2014-03-17 20:07, Walter Bright wrote:
>
>> ScopeBuffer has been there and commented on for about 2 months now.
>
> That's because you created a pull request instead of asking for a formal review.
> A pull request should NOT be created until a module is accepted after a review
> and voting.
>
> You're sneaking in a new module although it has not gone through a formal
> review. This is not how new modules should be added.

Internal modules do not need the formal review process, since they are not documented and not part of the public facing API, any more than other changes to the internals of Phobos functions need such review.

The review of ScopeBuffer on the github PR threads has been far more thorough than about any other. It's up to 260 comments on about 50 lines of actual code over about 2 months.

On Monday, 17 March 2014 at 20:06:49 UTC, Walter Bright wrote:
> On 3/17/2014 12:03 PM, monarch_dodra wrote:
>> So yes, it compiles, but the implementation is still wrong. Your implementation
>> allows assigning an immutable(int)* into an int*.
>>
>> Unless there is a performance specific reason for using the const, I don't see
>> why we aren't removing it.
>
> The reason was I was alternately calling put() with mutable char[] and immutable strings. I should look and see if there's a better way.

Ah... right. That makes sense. Templatising it is what usually makes the most sense, and most easilly solves the problem. Something like:
put(E)(E[] s)
if (is(typeof(buf[] = s)))

That said, given that:
1: You might be worrying about un-needed template bloat for types without indirections (such as strings)
2: You are only working with mutable types.

Then a static if should do the trick. Something like:
//----
static if (is(const(T) : T)
    alias CT = const(T);
else
    alias CT = T;

void put(T[] s)
{
    ...
    buf[used .. newlen] = s[];
}
//----
unittest
{
    ScopeBuffer!(int*) b;
    int*[] s;
    b.put(s);

    ScopeBuffer!char c;
    string s1;
    char[] s2;
    c.put(s1);
    c.put(s2);
}
/----

This works for me.

Does this change seem acceptable for you? Should I submit that?

On Monday, 17 March 2014 at 20:25:11 UTC, monarch_dodra wrote:
> Something like:
> put(E)(E[] s)
> if (is(typeof(buf[] = s)))

`buf[] = s[]`actually.


> static if (is(const(T) : T)
>     alias CT = const(T);
> else
>     alias CT = T;
>
> void put(T[] s)

This should read CT, of course.

I should have re-read.

On 3/17/2014 1:25 PM, monarch_dodra wrote:
> Does this change seem acceptable for you? Should I submit that?

Sure. And thanks.