www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Formal review of std.buffer.scopebuffer

reply "Dicebot" <public dicebot.lv> writes:
Initial announcement : 
http://forum.dlang.org/post/ld2586$17f6$1 digitalmars.com
Documentation        : 
http://www.walterbright.com/tmp/html/scopebuffer.html
Code                 : 
https://github.com/D-Programming-Language/phobos/pull/1911

Starting formal review thread for Walter's proposal.

Linked pull request already has some ongoing discussion and is 
worth investigating. In this thread please focus on those 
high-level properties of reviewed module:

  - Is it useful for standard library?
  - Does it fit into Phobos design?
  - Is documentation / API clear enough for you?
  - Will it be possible to keep it within its current API without 
breaking changes?
  - Are there any major oversights in existing implementation?

Smaller style / implementation comments are also welcome but less 
important at current stage.

Review will last for 2 weeks (ending on March 30) but can be 
prolonged upon explicit request.
Mar 16 2014
next sibling parent reply "David Nadlinger" <code klickverbot.at> writes:
On Sunday, 16 March 2014 at 19:53:53 UTC, Dicebot wrote:
 Initial announcement : 
 http://forum.dlang.org/post/ld2586$17f6$1 digitalmars.com
 Documentation        : 
 http://www.walterbright.com/tmp/html/scopebuffer.html
 Code                 : 
 https://github.com/D-Programming-Language/phobos/pull/1911

 Starting formal review thread for Walter's proposal.

Seems like the plan is to see how it works out for druntime/Phobos first: https://github.com/D-Programming-Language/druntime/pull/739. David
Mar 16 2014
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/16/14, 3:47 PM, David Eagen wrote:
 I've started using it in a project to replace a simple dynamic array
 buffer and so far it has fit my needs very well. One thing I ran into
 was a method in the project's API that has this signature:

 const(ubyte[]) foo() const

 I couldn't return a slice from ScopeBuffer directly because of the
 const. I resolved it by changing opSlice's signature in ScopeBuffer to
 this:

  system inout(T[]) opSlice() inout

 So there are two questions:

 1.) Is this a good way to resolve the problem or am I doing something
 horribly wrong?
 2.) Assuming this isn't horribly wrong should ScopeBuffer use inout like
 this?

good way, it should -- Andrei
Mar 16 2014
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Sunday, 16 March 2014 at 19:53:53 UTC, Dicebot wrote:
 Review will last for 2 weeks (ending on March 30) but can be 
 prolonged upon explicit request.

Actually I have just remembered that I won't have regular internet access on week after the next one so lets prolong it to April the 2nd :)
Mar 16 2014
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Sunday, 16 March 2014 at 20:08:32 UTC, David Nadlinger wrote:
 Seems like the plan is to see how it works out for 
 druntime/Phobos first: 
 https://github.com/D-Programming-Language/druntime/pull/739.

 David

I think that discussion happening there is much better suited to NG review thread, it is very important topic that should have full community exposure. Even if it does not fit into normal Phobos review limits (breaking the rules, yay!)
Mar 16 2014
prev sibling next sibling parent "David Eagen" <davideagen mailinator.com> writes:
I've started using it in a project to replace a simple dynamic 
array buffer and so far it has fit my needs very well. One thing 
I ran into was a method in the project's API that has this 
signature:

const(ubyte[]) foo() const

I couldn't return a slice from ScopeBuffer directly because of 
the const. I resolved it by changing opSlice's signature in 
ScopeBuffer to this:

 system inout(T[]) opSlice() inout

So there are two questions:

1.) Is this a good way to resolve the problem or am I doing 
something horribly wrong?
2.) Assuming this isn't horribly wrong should ScopeBuffer use 
inout like this?
Mar 16 2014
prev sibling next sibling parent Dmitry Olshansky <dmitry.olsh gmail.com> writes:
16-Mar-2014 23:53, Dicebot пишет:
 Initial announcement :
 http://forum.dlang.org/post/ld2586$17f6$1 digitalmars.com
 Documentation        :
 http://www.walterbright.com/tmp/html/scopebuffer.html
 Code                 :
 https://github.com/D-Programming-Language/phobos/pull/1911

 Starting formal review thread for Walter's proposal.

Seriously, there is no need for that now, since it was re-targeted for internal use only. -- Dmitry Olshansky
Mar 16 2014
prev sibling next sibling parent reply "Dicebot" <public dicebot.lv> writes:
Closing this seeing as Andrei has just merged it as std.internal 
into master. I am very angry about the way it has happened.
Mar 17 2014
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Mar 17 2014
parent reply Walter Bright <newshound2 digitalmars.com> writes:
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
Mar 17 2014
next sibling parent reply Jacob Carlborg <doob me.com> writes:
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
Mar 17 2014
next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
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.
Mar 17 2014
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/17/14, 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.

I had the same concerns about it being front and center in std. Now that it's internal that issue disappears - we can use it inside Phobos for a while and change it without disrupting users. In many ways putting it up for review after all makes things better for everybody. Andrei
Mar 17 2014
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/18/14, 12:45 AM, Jacob Carlborg wrote:
 On Monday, 17 March 2014 at 20:31:36 UTC, Andrei Alexandrescu wrote:

 I had the same concerns about it being front and center in std. Now
 that it's internal that issue disappears - we can use it inside Phobos
 for a while and change it without disrupting users. In many ways
 putting it up for review after all makes things better for everybody.

I don't think that issue disappears. Walter has done this before and now he did it again. Creating a pull request for a new module without asking for a formal review. It's less of a problem that it was merged as an internal module. The big issue trying to get everyone to follow the protocols we have for adding new modules.

I don't think we need a formal review process for internal modules. Proper review of the pull request should suffice. Andrei
Mar 18 2014
parent reply Jacob Carlborg <doob me.com> writes:
On 2014-03-18 16:59, Andrei Alexandrescu wrote:

 I don't think we need a formal review process for internal modules.
 Proper review of the pull request should suffice.

I agree. But this module _started out_ as a public module. If we have had a formal review we could have discussed and decided in the review that the module doesn't fit to be public. -- /Jacob Carlborg
Mar 18 2014
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/18/14, 1:53 PM, Jacob Carlborg wrote:
 On 2014-03-18 16:59, Andrei Alexandrescu wrote:

 I don't think we need a formal review process for internal modules.
 Proper review of the pull request should suffice.

I agree. But this module _started out_ as a public module. If we have had a formal review we could have discussed and decided in the review that the module doesn't fit to be public.

Instead we decided much quicker so it's all for the better. I really don't understand why the bickering about this still continues. Andrei
Mar 18 2014
prev sibling next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 3/18/2014 4:24 AM, Dicebot wrote:
 I was among those who has been continuously asking for cleaning Phobos
 allocations and I feel  that this modules about zero of issues I see. API
 allocations are much more important to fix than internal allocations and you
 still have not answered why you consider scopebuffer of more priority.

We can fix all the internal allocations without changing APIs and without any user disruption. This is low hanging fruit. Fixing external allocations will take longer and will have to be more carefully done.
 Also while it is important it is not at any hurry and shouldn't be done hastily
 simply because it is next discussion topic of the month.

Yes, it is important to not unnecessarily procrastinate on this. There are windows of opportunity for us. Moving the start of the window does not extend the end of it - it just means a shorter window of opportunity. Every day counts. Schedules slip one day at a time. We lose momentum every time there's a long Reddit thread about GC problems. We cannot afford this. Although the 2 month review of ScopeBuffer is far from hasty, we can be hasty about internal non-user-visible changes to Phobos because those can be easily backed out if they prove unworthy. There is nobody working on this besides me, other than the people working on nogc. (if I'm wrong, let me know). It's a high priority problem. I'm ok for me doing the work, but I ask for everyone's support in getting this done. --- In general, many issues with external GC allocations in Phobos can be resolved by changing the API so that the output goes to an OutputRange. std.string is a prime example of a module that needs to be range-ified. Any help with this would be important and appreciated. I know there are a couple people working on this with std.file, but there's plenty more needed. (For legacy compatibility, the old APIs must remain. I do not propose breaking existing code. But the old APIs can be redone as wrappers around the range versions.)
Mar 18 2014
prev sibling parent Walter Bright <newshound2 digitalmars.com> writes:
I neglected to mention that I do appreciate your concern about process, and 
thank you for being adamant about it.
Mar 18 2014
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
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.
Mar 17 2014
parent reply Walter Bright <newshound2 digitalmars.com> writes:
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.
Mar 17 2014
parent Walter Bright <newshound2 digitalmars.com> writes:
On 3/17/2014 1:25 PM, monarch_dodra wrote:
 Does this change seem acceptable for you? Should I submit that?

Sure. And thanks.
Mar 17 2014
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 17 March 2014 at 13:57:59 UTC, 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.

The fix for pointers hasn't been integrated. "ScopeBuff!(int*)" still doesn't compile.
Mar 17 2014
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
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.
Mar 17 2014
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
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.
Mar 17 2014
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
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?
Mar 17 2014
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
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.
Mar 17 2014
prev sibling next sibling parent "sybrandy" <sybrandy gmail.com> writes:
Apologies if this was answered elsewhere, but will this, or 
something similar, be exposed in Phobos for others to use?  This 
seems like something that could be quite useful depending on the 
application.  If not, why?
Mar 17 2014
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 17 March 2014 at 20:51:05 UTC, sybrandy wrote:
 Apologies if this was answered elsewhere, but will this, or 
 something similar, be exposed in Phobos for others to use?  
 This seems like something that could be quite useful depending 
 on the application.  If not, why?

FWIW, I've also begun working on something similar which (I think) would be more acceptable for "public" inclusion. In particular, that would provide better memory safety (subset usable in safe code), CTFE-able, and pure. And faster than Appender.
Mar 17 2014
prev sibling next sibling parent "Jacob Carlborg" <doob me.com> writes:
On Monday, 17 March 2014 at 20:20:46 UTC, Walter Bright wrote:

 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.

That's how it ended up being merged. That's not how it started. You created a pull request for a public visible new module without asking for a formal review. You have done this before. Please stop doing this.
 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.

Doesn't matter. Just follow the protocols. What's the problem with that? -- /Jacob Carlborg
Mar 18 2014
prev sibling next sibling parent "Jacob Carlborg" <doob me.com> writes:
On Monday, 17 March 2014 at 20:31:36 UTC, Andrei Alexandrescu 
wrote:

 I had the same concerns about it being front and center in std. 
 Now that it's internal that issue disappears - we can use it 
 inside Phobos for a while and change it without disrupting 
 users. In many ways putting it up for review after all makes 
 things better for everybody.

I don't think that issue disappears. Walter has done this before and now he did it again. Creating a pull request for a new module without asking for a formal review. It's less of a problem that it was merged as an internal module. The big issue trying to get everyone to follow the protocols we have for adding new modules. -- /Jacob Carlborg
Mar 18 2014
prev sibling next sibling parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Tuesday, 18 March 2014 at 07:45:03 UTC, Jacob Carlborg wrote:
 On Monday, 17 March 2014 at 20:31:36 UTC, Andrei Alexandrescu 
 wrote:

 I had the same concerns about it being front and center in 
 std. Now that it's internal that issue disappears - we can use 
 it inside Phobos for a while and change it without disrupting 
 users. In many ways putting it up for review after all makes 
 things better for everybody.

I don't think that issue disappears. Walter has done this before and now he did it again. Creating a pull request for a new module without asking for a formal review. It's less of a problem that it was merged as an internal module. The big issue trying to get everyone to follow the protocols we have for adding new modules.

In his defense, he *is* trying to keep our modules clean, which implies creating a new module. But let's put this into perspective: Said module is *1* type only. It's not like it was a new 10_000 line module or anything: It's 100 lines + unittest. If it was me, I would have just put the damn thing as a new type in "std.array", submitted it as a pull, with request for review nor mention in the boards, and then be done with it. And *then*, if we weren't happy about it, I would have marked it package too. It's not the first time we do this too. There's a fair bit in things in phobos that are "fairly useful, but don't justify public exposure". This is just one more of them. It just happens to be in a module. I'm not entirely happy about the way it went down, but I think we should cut him a little slack in that regards. That, and I think there was bad communication. On both sides.
Mar 18 2014
prev sibling parent "Dicebot" <public dicebot.lv> writes:
On Monday, 17 March 2014 at 19:07:44 UTC, Walter Bright wrote:
 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.

You see, review process exists for the very reason this is not clear even if it seems so. I will never take the courage of judging early termination of review simply because it does not seem to succeed. If anything, I'd try to encourage as much different input as possible to make decision that is clear to external observer. If you want something internal, you just go for it. If you want something public and reviewed, changing your mind few days after review request is not something that leaves a good impression. Consider how an outsider that does not read NG daily may see it. We always could have added something needed immediately as internal module _AND_ proceeded with review of possible higher level alternative than can fit public Phobos.
 The objective technical issues raised were all addressed, and 
 the immutable/const one was corrected and unittests added for 
 it before it was pulled.

Ok, I have probably not noticed that between the noise which leads us to...
 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.

 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 not something to be proud of. It has got that many comments because you started to argue against style comments and queries for some performance data. That is completely out of the line of normal PR review. It does not matter if you judgement is right or wrong here - such approach simply creates too much noise over things that are not truly important. This is also the reason why high-level review happens before creating the pull - hard to stay focused otherwise.
 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.

I was among those who has been continuously asking for cleaning Phobos allocations and I feel that this modules about zero of issues I see. API allocations are much more important to fix than internal allocations and you still have not answered why you consider scopebuffer of more priority. Also while it is important it is not at any hurry and shouldn't be done hastily simply because it is next discussion topic of the month.
Mar 18 2014