Thread overview
[phobos] [D-Programming-Language/phobos] 64c5e3: Fix std.container.SList.linearRemove's TakeRange-b...
Jul 09, 2012
GitHub
Jul 09, 2012
Masahiro Nakagawa
Jul 09, 2012
Jonathan M Davis
Jul 09, 2012
Masahiro Nakagawa
Jul 10, 2012
Masahiro Nakagawa
July 08, 2012
  Branch: refs/heads/master
  Home:   https://github.com/D-Programming-Language/phobos
  Commit: 64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
      https://github.com/D-Programming-Language/phobos/commit/64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
  Author: Revellian <matz@localhost.localdomain>
  Date:   2012-06-25 (Mon, 25 Jun 2012)

  Changed paths:
    M std/container.d

  Log Message:
  -----------
  Fix std.container.SList.linearRemove's TakeRange-bug (if the TakeRange starts at the
head of the SList, the whole list was removed instead of only the
specified number.)


  Commit: 0cae7ca6af24f6ad322edbe263f43f88b5c5650a
      https://github.com/D-Programming-Language/phobos/commit/0cae7ca6af24f6ad322edbe263f43f88b5c5650a
  Author: Revellian <matz@localhost.localdomain>
  Date:   2012-06-25 (Mon, 25 Jun 2012)

  Changed paths:
    M std/container.d

  Log Message:
  -----------
  Added doubly-linked list (DList) to std.container


  Commit: 61fa7c809cd4db2e214f5f2702690bd52436b7a0
      https://github.com/D-Programming-Language/phobos/commit/61fa7c809cd4db2e214f5f2702690bd52436b7a0
  Author: Revellian <matz@localhost.localdomain>
  Date:   2012-07-02 (Mon, 02 Jul 2012)

  Changed paths:
    M std/container.d

  Log Message:
  -----------
  - Incorporate comments by andralex


  Commit: e0757c5ded416d7c781a8199ae2f73eab7da3dc6
      https://github.com/D-Programming-Language/phobos/commit/e0757c5ded416d7c781a8199ae2f73eab7da3dc6
  Author: Andrei Alexandrescu <andrei@erdani.com>
  Date:   2012-07-08 (Sun, 08 Jul 2012)

  Changed paths:
    M std/container.d

  Log Message:
  -----------
  Merge pull request #650 from revellian/master

Fix std.container.SList.linearRemove's TakeRange-bug


Compare: https://github.com/D-Programming-Language/phobos/compare/865c7a6f9d00...e0757c5ded41


July 09, 2012
I was a little surprised.
I didn't know DList proposal until after I read this mail.
Why did this pull request mix with fixing and adding new feature?


On Mon, Jul 9, 2012 at 12:07 PM, GitHub <noreply@github.com> wrote:
>   Branch: refs/heads/master
>   Home:   https://github.com/D-Programming-Language/phobos
>   Commit: 64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
>       https://github.com/D-Programming-Language/phobos/commit/64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
>   Author: Revellian <matz@localhost.localdomain>
>   Date:   2012-06-25 (Mon, 25 Jun 2012)
>
>   Changed paths:
>     M std/container.d
>
>   Log Message:
>   -----------
>   Fix std.container.SList.linearRemove's TakeRange-bug (if the TakeRange starts at the
> head of the SList, the whole list was removed instead of only the
> specified number.)
>
>
>   Commit: 0cae7ca6af24f6ad322edbe263f43f88b5c5650a
>       https://github.com/D-Programming-Language/phobos/commit/0cae7ca6af24f6ad322edbe263f43f88b5c5650a
>   Author: Revellian <matz@localhost.localdomain>
>   Date:   2012-06-25 (Mon, 25 Jun 2012)
>
>   Changed paths:
>     M std/container.d
>
>   Log Message:
>   -----------
>   Added doubly-linked list (DList) to std.container
>
>
>   Commit: 61fa7c809cd4db2e214f5f2702690bd52436b7a0
>       https://github.com/D-Programming-Language/phobos/commit/61fa7c809cd4db2e214f5f2702690bd52436b7a0
>   Author: Revellian <matz@localhost.localdomain>
>   Date:   2012-07-02 (Mon, 02 Jul 2012)
>
>   Changed paths:
>     M std/container.d
>
>   Log Message:
>   -----------
>   - Incorporate comments by andralex
>
>
>   Commit: e0757c5ded416d7c781a8199ae2f73eab7da3dc6
>       https://github.com/D-Programming-Language/phobos/commit/e0757c5ded416d7c781a8199ae2f73eab7da3dc6
>   Author: Andrei Alexandrescu <andrei@erdani.com>
>   Date:   2012-07-08 (Sun, 08 Jul 2012)
>
>   Changed paths:
>     M std/container.d
>
>   Log Message:
>   -----------
>   Merge pull request #650 from revellian/master
>
> Fix std.container.SList.linearRemove's TakeRange-bug
>
>
> Compare: https://github.com/D-Programming-Language/phobos/compare/865c7a6f9d00...e0757c5ded41
>
> _______________________________________________
> phobos mailing list
> phobos@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

July 08, 2012
On Monday, July 09, 2012 13:33:53 Masahiro Nakagawa wrote:
> I was a little surprised.
> I didn't know DList proposal until after I read this mail.
> Why did this pull request mix with fixing and adding new feature?

Because for some reason the developer who created the pull request mixed them, and Andrei didn't insist on them being separated before merging them. I'd originally told the developer to separate them, but he didn't, and after he worked out the issues with DList with Andrei, Andrei merged it, so they never got separated. It's not a big deal that they got merged together as long as they're both good, but they really should have been separate pull requests in the first place.

- Jonathan M Davis
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

July 09, 2012
Yah, the title doesn't reflect it. But a DList is an obvious continuation of std.container and I just didn't care about it being a separate pull request. The design of DList is fairly fixed, there's little need to e.g. have a formal review for it.

Of course adding allocators will affect DList and all other containers, but I hope in a rather uniform way.


Andrei

On 7/9/12 12:33 AM, Masahiro Nakagawa wrote:
> I was a little surprised.
> I didn't know DList proposal until after I read this mail.
> Why did this pull request mix with fixing and adding new feature?
>
>
> On Mon, Jul 9, 2012 at 12:07 PM, GitHub<noreply@github.com>  wrote:
>>    Branch: refs/heads/master
>>    Home:   https://github.com/D-Programming-Language/phobos
>>    Commit: 64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
>>        https://github.com/D-Programming-Language/phobos/commit/64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
>>    Author: Revellian<matz@localhost.localdomain>
>>    Date:   2012-06-25 (Mon, 25 Jun 2012)
>>
>>    Changed paths:
>>      M std/container.d
>>
>>    Log Message:
>>    -----------
>>    Fix std.container.SList.linearRemove's TakeRange-bug (if the TakeRange starts at the
>> head of the SList, the whole list was removed instead of only the
>> specified number.)
>>
>>
>>    Commit: 0cae7ca6af24f6ad322edbe263f43f88b5c5650a
>>        https://github.com/D-Programming-Language/phobos/commit/0cae7ca6af24f6ad322edbe263f43f88b5c5650a
>>    Author: Revellian<matz@localhost.localdomain>
>>    Date:   2012-06-25 (Mon, 25 Jun 2012)
>>
>>    Changed paths:
>>      M std/container.d
>>
>>    Log Message:
>>    -----------
>>    Added doubly-linked list (DList) to std.container
>>
>>
>>    Commit: 61fa7c809cd4db2e214f5f2702690bd52436b7a0
>>        https://github.com/D-Programming-Language/phobos/commit/61fa7c809cd4db2e214f5f2702690bd52436b7a0
>>    Author: Revellian<matz@localhost.localdomain>
>>    Date:   2012-07-02 (Mon, 02 Jul 2012)
>>
>>    Changed paths:
>>      M std/container.d
>>
>>    Log Message:
>>    -----------
>>    - Incorporate comments by andralex
>>
>>
>>    Commit: e0757c5ded416d7c781a8199ae2f73eab7da3dc6
>>        https://github.com/D-Programming-Language/phobos/commit/e0757c5ded416d7c781a8199ae2f73eab7da3dc6
>>    Author: Andrei Alexandrescu<andrei@erdani.com>
>>    Date:   2012-07-08 (Sun, 08 Jul 2012)
>>
>>    Changed paths:
>>      M std/container.d
>>
>>    Log Message:
>>    -----------
>>    Merge pull request #650 from revellian/master
>>
>> Fix std.container.SList.linearRemove's TakeRange-bug
>>
>>
>> Compare: https://github.com/D-Programming-Language/phobos/compare/865c7a6f9d00...e0757c5ded41
>>
>> _______________________________________________
>> phobos mailing list
>> phobos@puremagic.com
>> http://lists.puremagic.com/mailman/listinfo/phobos
> _______________________________________________
> phobos mailing list
> phobos@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

July 09, 2012
Jonathan,

Thank you for the clarification!
I understood the situation.

We should check the mixed state of pull request.
Clear title and content make management more easy and
other member can attend the discussion :)


On Mon, Jul 9, 2012 at 1:38 PM, Jonathan M Davis <jmdavisProg@gmx.com> wrote:
> On Monday, July 09, 2012 13:33:53 Masahiro Nakagawa wrote:
>> I was a little surprised.
>> I didn't know DList proposal until after I read this mail.
>> Why did this pull request mix with fixing and adding new feature?
>
> Because for some reason the developer who created the pull request mixed them, and Andrei didn't insist on them being separated before merging them. I'd originally told the developer to separate them, but he didn't, and after he worked out the issues with DList with Andrei, Andrei merged it, so they never got separated. It's not a big deal that they got merged together as long as they're both good, but they really should have been separate pull requests in the first place.
>
> - Jonathan M Davis
> _______________________________________________
> phobos mailing list
> phobos@puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

July 10, 2012
I don't know why, but I sent following post to Andrei directly...

We may have to define the rough rule of pull request.
If title hides the commits, the state of pull request is hard to know.
Separating pull requests are searchable and manageable.


Masahiro

On Mon, Jul 9, 2012 at 2:00 PM, Andrei Alexandrescu <andrei@erdani.com> wrote:
> Yah, the title doesn't reflect it. But a DList is an obvious continuation of std.container and I just didn't care about it being a separate pull request. The design of DList is fairly fixed, there's little need to e.g. have a formal review for it.
>
> Of course adding allocators will affect DList and all other containers, but I hope in a rather uniform way.
>
>
> Andrei
>
> On 7/9/12 12:33 AM, Masahiro Nakagawa wrote:
>>
>> I was a little surprised.
>> I didn't know DList proposal until after I read this mail.
>> Why did this pull request mix with fixing and adding new feature?
>>
>>
>> On Mon, Jul 9, 2012 at 12:07 PM, GitHub<noreply@github.com>  wrote:
>>>
>>>    Branch: refs/heads/master
>>>    Home:   https://github.com/D-Programming-Language/phobos
>>>    Commit: 64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
>>>
>>> https://github.com/D-Programming-Language/phobos/commit/64c5e3cffa58fecdb8ecb23fd175e5b8dcede2a7
>>>    Author: Revellian<matz@localhost.localdomain>
>>>    Date:   2012-06-25 (Mon, 25 Jun 2012)
>>>
>>>    Changed paths:
>>>      M std/container.d
>>>
>>>    Log Message:
>>>    -----------
>>>    Fix std.container.SList.linearRemove's TakeRange-bug (if the TakeRange
>>> starts at the
>>> head of the SList, the whole list was removed instead of only the
>>> specified number.)
>>>
>>>
>>>    Commit: 0cae7ca6af24f6ad322edbe263f43f88b5c5650a
>>>
>>> https://github.com/D-Programming-Language/phobos/commit/0cae7ca6af24f6ad322edbe263f43f88b5c5650a
>>>    Author: Revellian<matz@localhost.localdomain>
>>>    Date:   2012-06-25 (Mon, 25 Jun 2012)
>>>
>>>    Changed paths:
>>>      M std/container.d
>>>
>>>    Log Message:
>>>    -----------
>>>    Added doubly-linked list (DList) to std.container
>>>
>>>
>>>    Commit: 61fa7c809cd4db2e214f5f2702690bd52436b7a0
>>>
>>> https://github.com/D-Programming-Language/phobos/commit/61fa7c809cd4db2e214f5f2702690bd52436b7a0
>>>    Author: Revellian<matz@localhost.localdomain>
>>>    Date:   2012-07-02 (Mon, 02 Jul 2012)
>>>
>>>    Changed paths:
>>>      M std/container.d
>>>
>>>    Log Message:
>>>    -----------
>>>    - Incorporate comments by andralex
>>>
>>>
>>>    Commit: e0757c5ded416d7c781a8199ae2f73eab7da3dc6
>>>
>>> https://github.com/D-Programming-Language/phobos/commit/e0757c5ded416d7c781a8199ae2f73eab7da3dc6
>>>    Author: Andrei Alexandrescu<andrei@erdani.com>
>>>    Date:   2012-07-08 (Sun, 08 Jul 2012)
>>>
>>>    Changed paths:
>>>      M std/container.d
>>>
>>>    Log Message:
>>>    -----------
>>>    Merge pull request #650 from revellian/master
>>>
>>> Fix std.container.SList.linearRemove's TakeRange-bug
>>>
>>>
>>> Compare: https://github.com/D-Programming-Language/phobos/compare/865c7a6f9d00...e0757c5ded41
>>>
>>> _______________________________________________
>>> phobos mailing list
>>> phobos@puremagic.com
>>> http://lists.puremagic.com/mailman/listinfo/phobos
>>
>> _______________________________________________
>> phobos mailing list
>> phobos@puremagic.com
>> http://lists.puremagic.com/mailman/listinfo/phobos
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos

July 09, 2012
On 7/9/12 9:17 PM, Masahiro Nakagawa wrote:
> We may have to define the rough rule of pull request.
> If title hides the commits, the state of pull request is hard to know.
> Separating pull requests are searchable and manageable.

Agreed. Let's be more proactive about this in the future.

Andrei
_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos