On Tuesday, August 16, 2011 17:08:26 David Nadlinger wrote:
> Seems like the reviews are already coming in steadily, but we still have no review manager – if nobody else steps up until tomorrow, I'd volunteer so we can get the formal review process going.

I'd volunteered previously before std.path got reviewed, but there was no response, so std.path got reviewed first. So, I can do it, but it's fine with me if you do it. I only volunteered before in attempt to get it moving, which didn't happen at the time.

- Jonathan MDavis

On 2011-08-16 22:01, Jonathan M Davis wrote:
> On Tuesday, August 16, 2011 18:47:44 Jacob Carlborg wrote:
>> BTW, do we have style guides for abbreviations like this?
>
> No. The style guide says camelcase, but it doesn't specify how to deal with
> abbreviations. Personally, I think that it should be dealt with on a case by
> case basis, since what works best depends on the abbreviation, but in general,
> I prefer all caps.
>
> - Jonathan M Davis

There's also the question when having abbreviations in a method name:

sendToHTTP

or

sendToHttp

In general think abbreviations should be avoided except when the abbreviation is better known, e.g. HTTP, FTP and so on. I like camlecase better than all caps both for class names and method names.

-- 
/Jacob Carlborg

On Wednesday, August 17, 2011 09:01:02 Jacob Carlborg wrote:
> On 2011-08-16 22:01, Jonathan M Davis wrote:
> > On Tuesday, August 16, 2011 18:47:44 Jacob Carlborg wrote:
> >> BTW, do we have style guides for abbreviations like this?
> > 
> > No. The style guide says camelcase, but it doesn't specify how to deal with abbreviations. Personally, I think that it should be dealt with on a case by case basis, since what works best depends on the abbreviation, but in general, I prefer all caps.
> > 
> > - Jonathan M Davis
> 
> There's also the question when having abbreviations in a method name:
> 
> sendToHTTP
> 
> or
> 
> sendToHttp
> 
> In general think abbreviations should be avoided except when the abbreviation is better known, e.g. HTTP, FTP and so on. I like camlecase better than all caps both for class names and method names.

I meant that I generally prefer all caps for the abbreviations (e.g. sentToHTTP rather than sendToHttp). I definitely think that the function names should be camelcased and the types should be pascal cased - which is what the d style is. And yes, abbreviations should only be used when they're appropriately clear. Function names should be as short as we can reasonably make them and still have them be clear, but clarity ranks higher than brevity when it comes to picking good function names IMHO.

- Jonathan M Davis

On 16/08/11 22.39, bearophile wrote:
> jdrewsen:
>
>> etc.c.curl is already in phobos and no binary library for curl is in.
>
> I see. I didn't know this. Then it's the good moment to improve this situation.
>
>
>> I disagree on this one though. But please see my reply to jonathan as to
>> how I see final solution.
>
> Most times I'll not use a Phobos module that to be used requires the compilation of a multi-file C library. I use another language where this functionality is already present, like this one:
> http://docs.python.org/library/httplib.html

If a decent package management tool was added to D which could handle downloading the binary lib for you then wouldn't that be an okey solition for you?

> Accessing the web from the language today is a basic feature, and quite more commonly needed than as example complex numbers.
>
> In Clojure even the Phobos equivalent of the File function loads from the web if you give it an URL instead  of a file path :-) Please help pushing D into this decade.

I agree that this functionality should be a standard part of phobos (ie. the 'std' package). It just has to be implemented in D and not just be a wrapper. The way I would like it is to initially get a reactor/proactor implemented into D and then on top of that a nice http/ftp/... client and server API.

Wrappers should be handled by a package manager in my opinion.

/Jonas

> Bye,
> bearophile

On 17/08/11 00.21, Jonathan M Davis wrote:
> On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
>> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen<jdrewsen@nospam.com>  wrote:
>>> Den 16-08-2011 18:55, Martin Nowak skrev:
>>>> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha<dsimcha@yahoo.com>  wrote:
>>>>> On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This is a review request for the curl wrapper. Please read the "known
>>>>>> issues" in the top of the source file and if possible suggest a
>>>>>> solution.
>>>>>>
>>>>>> We also need somebody for running the review process. Anyone?
>>>>>>
>>>>>> Code:
>>>>>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>>>>>> Docs:
>>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>>>>
>>>>>> Demolish!
>>>>>>
>>>>>> /Jonas
>>>>>
>>>>>  From a quick look, this looks very well thought out. I'll review it
>>>>> more thoroughly when I have more time. A few questions/comments from a
>>>>> quick look at the docs:
>>>>>
>>>>> Does the async stuff use D threads, or does Curl have its own async
>>>>> API?
>>>>>
>>>>> In your examples for postData, you have onReceive a ubyte[] and write
>>>>> it out to console. Did you mean to cast this to some kind of string?
>>>>>
>>>>> For onReceive, what's the purpose of the return value?
>>>>>
>>>>> If/when this module makes it into Phobos, are we going to start
>>>>> including a libcurl binary with DMD distributions so that std.curl
>>>>> feels truly **standard** and requires zero extra configuration?
>>>>
>>>> I was also wondering about the async handling. In the long-term I'd like
>>>> to see a bigger picture for async handling in phobos (offering some kind
>>>> of futures, maybe event-loops etc.).
>>>> Though this is not a requirement for the curl wrapper now.
>>>> std.parallelism also has some kind of this stuff and file reading would
>>>> benefit from it too.
>>>
>>> This has been discussed before and I also think this is very important.
>>> But before that I think some kind of package management should be
>>> prioritized (A DIP11 implementaion or a more traditional solution).
>>>
>>>> One thing I spotted at a quick glance, sending to be filled buffers to
>>>> another thread should not be done by casting to shared not immutable.
>>>
>>> I'm not entirely sure what you mean. There is no use of shared buffers
>>> in the wrapper. I do cast the buffer between mutable/immutable because
>>> only immutable or by value data can be passed using std.concurrency.
>>> Since the buffers are only used by the thread that currently has the
>>> buffer this is safe. I've previously asked for a non-cast solution (ie.
>>> some kind of move between threads semantic for std.concurrency) but was
>>> advised that this was the way to do it.
>>>
>>>> martin
>>
>> Pardon the typo. What I meant is that AFAIK casting from immutable to
>> mutable has undefined behavior.
>> The intended method for sending a uint[] buffer to another thread is to
>> cast that
>> buffer to shared (cast(shared(uint[])) and casting away the shared on the
>> receiving side.
>> It is allowed to send shared data using std.concurrency.
>
> Casting away immutability and then altering data is undefined. Actually
> casting it away is defined. So, if you have data in one thread that you know
> is unique, you can cast it to immutable (or std.exception.assumeUnique to do
> it) and then send it to another thread. On that thread, you can then cast it
> to mutable and alter it.
>
> However, you're circumventing the type system when you do this. So, you have
> to be very careful. You're throwing away the guarantees that the compiler
> makes with regards to const and immutable. It _is_ guaranteed to work though.
> And I'm not sure that there's really any difference between casting to shared
> and back and casting to immutable and back. In both cases, you're
> circumventing the type system. The main difference would be that if you
> screwed up with immutable and cast away immutable on something that really was
> immutable rather than something that you cast to immutable just to send it to
> another thread, then you could a segfault when you tried to alter it, since it
> could be in ROM.
>
> - Jonathan M Davis

Yeah I know you have to be careful when doing these kind of things. I ran into the problem of sending buffers between threads (using std.concurrency) so that they could be reused. There isn't any "move ownership" support in place so Andrei suggested I could do it by casting immutable.

If anyone knows of a cleaner way to do this please tell.

/Jonas

On Wed, 17 Aug 2011 05:43:00 -0400, Jonas Drewsen <jdrewsen@nospam.com> wrote:

> On 17/08/11 00.21, Jonathan M Davis wrote:
>> On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
>>> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen<jdrewsen@nospam.com>  wrote:
>>>> Den 16-08-2011 18:55, Martin Nowak skrev:
>>>>> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha<dsimcha@yahoo.com>  wrote:
>>>>>> On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> This is a review request for the curl wrapper. Please read the "known
>>>>>>> issues" in the top of the source file and if possible suggest a
>>>>>>> solution.
>>>>>>>
>>>>>>> We also need somebody for running the review process. Anyone?
>>>>>>>
>>>>>>> Code:
>>>>>>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>>>>>>> Docs:
>>>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>>>>>
>>>>>>> Demolish!
>>>>>>>
>>>>>>> /Jonas
>>>>>>
>>>>>>  From a quick look, this looks very well thought out. I'll review it
>>>>>> more thoroughly when I have more time. A few questions/comments from a
>>>>>> quick look at the docs:
>>>>>>
>>>>>> Does the async stuff use D threads, or does Curl have its own async
>>>>>> API?
>>>>>>
>>>>>> In your examples for postData, you have onReceive a ubyte[] and write
>>>>>> it out to console. Did you mean to cast this to some kind of string?
>>>>>>
>>>>>> For onReceive, what's the purpose of the return value?
>>>>>>
>>>>>> If/when this module makes it into Phobos, are we going to start
>>>>>> including a libcurl binary with DMD distributions so that std.curl
>>>>>> feels truly **standard** and requires zero extra configuration?
>>>>>
>>>>> I was also wondering about the async handling. In the long-term I'd like
>>>>> to see a bigger picture for async handling in phobos (offering some kind
>>>>> of futures, maybe event-loops etc.).
>>>>> Though this is not a requirement for the curl wrapper now.
>>>>> std.parallelism also has some kind of this stuff and file reading would
>>>>> benefit from it too.
>>>>
>>>> This has been discussed before and I also think this is very important.
>>>> But before that I think some kind of package management should be
>>>> prioritized (A DIP11 implementaion or a more traditional solution).
>>>>
>>>>> One thing I spotted at a quick glance, sending to be filled buffers to
>>>>> another thread should not be done by casting to shared not immutable.
>>>>
>>>> I'm not entirely sure what you mean. There is no use of shared buffers
>>>> in the wrapper. I do cast the buffer between mutable/immutable because
>>>> only immutable or by value data can be passed using std.concurrency.
>>>> Since the buffers are only used by the thread that currently has the
>>>> buffer this is safe. I've previously asked for a non-cast solution (ie.
>>>> some kind of move between threads semantic for std.concurrency) but was
>>>> advised that this was the way to do it.
>>>>
>>>>> martin
>>>
>>> Pardon the typo. What I meant is that AFAIK casting from immutable to
>>> mutable has undefined behavior.
>>> The intended method for sending a uint[] buffer to another thread is to
>>> cast that
>>> buffer to shared (cast(shared(uint[])) and casting away the shared on the
>>> receiving side.
>>> It is allowed to send shared data using std.concurrency.
>>
>> Casting away immutability and then altering data is undefined. Actually
>> casting it away is defined. So, if you have data in one thread that you know
>> is unique, you can cast it to immutable (or std.exception.assumeUnique to do
>> it) and then send it to another thread. On that thread, you can then cast it
>> to mutable and alter it.
>>
>> However, you're circumventing the type system when you do this. So, you have
>> to be very careful. You're throwing away the guarantees that the compiler
>> makes with regards to const and immutable. It _is_ guaranteed to work though.
>> And I'm not sure that there's really any difference between casting to shared
>> and back and casting to immutable and back. In both cases, you're
>> circumventing the type system. The main difference would be that if you
>> screwed up with immutable and cast away immutable on something that really was
>> immutable rather than something that you cast to immutable just to send it to
>> another thread, then you could a segfault when you tried to alter it, since it
>> could be in ROM.
>>
>> - Jonathan M Davis
>
> Yeah I know you have to be careful when doing these kind of things. I ran into the problem of sending buffers between threads (using std.concurrency) so that they could be reused. There isn't any "move ownership" support in place so Andrei suggested I could do it by casting immutable.
>
> If anyone knows of a cleaner way to do this please tell.

casting to shared and back.  Passing shared data should be supported by std.concurrency, and casting away shared is defined as long as you know only one thread owns the data after casting.

-Steve

Den 17-08-2011 15:51, Steven Schveighoffer skrev:
> On Wed, 17 Aug 2011 05:43:00 -0400, Jonas Drewsen <jdrewsen@nospam.com>
> wrote:
>
>> On 17/08/11 00.21, Jonathan M Davis wrote:
>>> On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
>>>> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen<jdrewsen@nospam.com>
>>>> wrote:
>>>>> Den 16-08-2011 18:55, Martin Nowak skrev:
>>>>>> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha<dsimcha@yahoo.com> wrote:
>>>>>>> On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> This is a review request for the curl wrapper. Please read the
>>>>>>>> "known
>>>>>>>> issues" in the top of the source file and if possible suggest a
>>>>>>>> solution.
>>>>>>>>
>>>>>>>> We also need somebody for running the review process. Anyone?
>>>>>>>>
>>>>>>>> Code:
>>>>>>>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>>>>>>>> Docs:
>>>>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>>>>>>
>>>>>>>> Demolish!
>>>>>>>>
>>>>>>>> /Jonas
>>>>>>>
>>>>>>> From a quick look, this looks very well thought out. I'll review it
>>>>>>> more thoroughly when I have more time. A few questions/comments
>>>>>>> from a
>>>>>>> quick look at the docs:
>>>>>>>
>>>>>>> Does the async stuff use D threads, or does Curl have its own async
>>>>>>> API?
>>>>>>>
>>>>>>> In your examples for postData, you have onReceive a ubyte[] and
>>>>>>> write
>>>>>>> it out to console. Did you mean to cast this to some kind of string?
>>>>>>>
>>>>>>> For onReceive, what's the purpose of the return value?
>>>>>>>
>>>>>>> If/when this module makes it into Phobos, are we going to start
>>>>>>> including a libcurl binary with DMD distributions so that std.curl
>>>>>>> feels truly **standard** and requires zero extra configuration?
>>>>>>
>>>>>> I was also wondering about the async handling. In the long-term
>>>>>> I'd like
>>>>>> to see a bigger picture for async handling in phobos (offering
>>>>>> some kind
>>>>>> of futures, maybe event-loops etc.).
>>>>>> Though this is not a requirement for the curl wrapper now.
>>>>>> std.parallelism also has some kind of this stuff and file reading
>>>>>> would
>>>>>> benefit from it too.
>>>>>
>>>>> This has been discussed before and I also think this is very
>>>>> important.
>>>>> But before that I think some kind of package management should be
>>>>> prioritized (A DIP11 implementaion or a more traditional solution).
>>>>>
>>>>>> One thing I spotted at a quick glance, sending to be filled
>>>>>> buffers to
>>>>>> another thread should not be done by casting to shared not immutable.
>>>>>
>>>>> I'm not entirely sure what you mean. There is no use of shared buffers
>>>>> in the wrapper. I do cast the buffer between mutable/immutable because
>>>>> only immutable or by value data can be passed using std.concurrency.
>>>>> Since the buffers are only used by the thread that currently has the
>>>>> buffer this is safe. I've previously asked for a non-cast solution
>>>>> (ie.
>>>>> some kind of move between threads semantic for std.concurrency) but
>>>>> was
>>>>> advised that this was the way to do it.
>>>>>
>>>>>> martin
>>>>
>>>> Pardon the typo. What I meant is that AFAIK casting from immutable to
>>>> mutable has undefined behavior.
>>>> The intended method for sending a uint[] buffer to another thread is to
>>>> cast that
>>>> buffer to shared (cast(shared(uint[])) and casting away the shared
>>>> on the
>>>> receiving side.
>>>> It is allowed to send shared data using std.concurrency.
>>>
>>> Casting away immutability and then altering data is undefined. Actually
>>> casting it away is defined. So, if you have data in one thread that
>>> you know
>>> is unique, you can cast it to immutable (or
>>> std.exception.assumeUnique to do
>>> it) and then send it to another thread. On that thread, you can then
>>> cast it
>>> to mutable and alter it.
>>>
>>> However, you're circumventing the type system when you do this. So,
>>> you have
>>> to be very careful. You're throwing away the guarantees that the
>>> compiler
>>> makes with regards to const and immutable. It _is_ guaranteed to work
>>> though.
>>> And I'm not sure that there's really any difference between casting
>>> to shared
>>> and back and casting to immutable and back. In both cases, you're
>>> circumventing the type system. The main difference would be that if you
>>> screwed up with immutable and cast away immutable on something that
>>> really was
>>> immutable rather than something that you cast to immutable just to
>>> send it to
>>> another thread, then you could a segfault when you tried to alter it,
>>> since it
>>> could be in ROM.
>>>
>>> - Jonathan M Davis
>>
>> Yeah I know you have to be careful when doing these kind of things. I
>> ran into the problem of sending buffers between threads (using
>> std.concurrency) so that they could be reused. There isn't any "move
>> ownership" support in place so Andrei suggested I could do it by
>> casting immutable.
>>
>> If anyone knows of a cleaner way to do this please tell.
>
> casting to shared and back. Passing shared data should be supported by
> std.concurrency, and casting away shared is defined as long as you know
> only one thread owns the data after casting.
>
> -Steve

Why is this cleaner than casting to immutable and back?

/Jonas

On 08/17/2011 05:05 PM, jdrewsen wrote:
> Den 17-08-2011 15:51, Steven Schveighoffer skrev:
>> On Wed, 17 Aug 2011 05:43:00 -0400, Jonas Drewsen <jdrewsen@nospam.com>
>> wrote:
>>
>>> On 17/08/11 00.21, Jonathan M Davis wrote:
>>>> On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
>>>>> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen<jdrewsen@nospam.com>
>>>>> wrote:
>>>>>> Den 16-08-2011 18:55, Martin Nowak skrev:
>>>>>>> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha<dsimcha@yahoo.com>
>>>>>>> wrote:
>>>>>>>> On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> This is a review request for the curl wrapper. Please read the
>>>>>>>>> "known
>>>>>>>>> issues" in the top of the source file and if possible suggest a
>>>>>>>>> solution.
>>>>>>>>>
>>>>>>>>> We also need somebody for running the review process. Anyone?
>>>>>>>>>
>>>>>>>>> Code:
>>>>>>>>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>>>>>>>>> Docs:
>>>>>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>>>>>>>
>>>>>>>>> Demolish!
>>>>>>>>>
>>>>>>>>> /Jonas
>>>>>>>>
>>>>>>>> From a quick look, this looks very well thought out. I'll review it
>>>>>>>> more thoroughly when I have more time. A few questions/comments
>>>>>>>> from a
>>>>>>>> quick look at the docs:
>>>>>>>>
>>>>>>>> Does the async stuff use D threads, or does Curl have its own async
>>>>>>>> API?
>>>>>>>>
>>>>>>>> In your examples for postData, you have onReceive a ubyte[] and
>>>>>>>> write
>>>>>>>> it out to console. Did you mean to cast this to some kind of
>>>>>>>> string?
>>>>>>>>
>>>>>>>> For onReceive, what's the purpose of the return value?
>>>>>>>>
>>>>>>>> If/when this module makes it into Phobos, are we going to start
>>>>>>>> including a libcurl binary with DMD distributions so that std.curl
>>>>>>>> feels truly **standard** and requires zero extra configuration?
>>>>>>>
>>>>>>> I was also wondering about the async handling. In the long-term
>>>>>>> I'd like
>>>>>>> to see a bigger picture for async handling in phobos (offering
>>>>>>> some kind
>>>>>>> of futures, maybe event-loops etc.).
>>>>>>> Though this is not a requirement for the curl wrapper now.
>>>>>>> std.parallelism also has some kind of this stuff and file reading
>>>>>>> would
>>>>>>> benefit from it too.
>>>>>>
>>>>>> This has been discussed before and I also think this is very
>>>>>> important.
>>>>>> But before that I think some kind of package management should be
>>>>>> prioritized (A DIP11 implementaion or a more traditional solution).
>>>>>>
>>>>>>> One thing I spotted at a quick glance, sending to be filled
>>>>>>> buffers to
>>>>>>> another thread should not be done by casting to shared not
>>>>>>> immutable.
>>>>>>
>>>>>> I'm not entirely sure what you mean. There is no use of shared
>>>>>> buffers
>>>>>> in the wrapper. I do cast the buffer between mutable/immutable
>>>>>> because
>>>>>> only immutable or by value data can be passed using std.concurrency.
>>>>>> Since the buffers are only used by the thread that currently has the
>>>>>> buffer this is safe. I've previously asked for a non-cast solution
>>>>>> (ie.
>>>>>> some kind of move between threads semantic for std.concurrency) but
>>>>>> was
>>>>>> advised that this was the way to do it.
>>>>>>
>>>>>>> martin
>>>>>
>>>>> Pardon the typo. What I meant is that AFAIK casting from immutable to
>>>>> mutable has undefined behavior.
>>>>> The intended method for sending a uint[] buffer to another thread
>>>>> is to
>>>>> cast that
>>>>> buffer to shared (cast(shared(uint[])) and casting away the shared
>>>>> on the
>>>>> receiving side.
>>>>> It is allowed to send shared data using std.concurrency.
>>>>
>>>> Casting away immutability and then altering data is undefined. Actually
>>>> casting it away is defined. So, if you have data in one thread that
>>>> you know
>>>> is unique, you can cast it to immutable (or
>>>> std.exception.assumeUnique to do
>>>> it) and then send it to another thread. On that thread, you can then
>>>> cast it
>>>> to mutable and alter it.
>>>>
>>>> However, you're circumventing the type system when you do this. So,
>>>> you have
>>>> to be very careful. You're throwing away the guarantees that the
>>>> compiler
>>>> makes with regards to const and immutable. It _is_ guaranteed to work
>>>> though.
>>>> And I'm not sure that there's really any difference between casting
>>>> to shared
>>>> and back and casting to immutable and back. In both cases, you're
>>>> circumventing the type system. The main difference would be that if you
>>>> screwed up with immutable and cast away immutable on something that
>>>> really was
>>>> immutable rather than something that you cast to immutable just to
>>>> send it to
>>>> another thread, then you could a segfault when you tried to alter it,
>>>> since it
>>>> could be in ROM.
>>>>
>>>> - Jonathan M Davis
>>>
>>> Yeah I know you have to be careful when doing these kind of things. I
>>> ran into the problem of sending buffers between threads (using
>>> std.concurrency) so that they could be reused. There isn't any "move
>>> ownership" support in place so Andrei suggested I could do it by
>>> casting immutable.
>>>
>>> If anyone knows of a cleaner way to do this please tell.
>>
>> casting to shared and back. Passing shared data should be supported by
>> std.concurrency, and casting away shared is defined as long as you know
>> only one thread owns the data after casting.
>>
>> -Steve
>
> Why is this cleaner than casting to immutable and back?
>
> /Jonas
>
>
>

Because immutable gives strictly stronger guarantees than shared. Casting away immutable and altering the resulting data is incorrect, even if it works with a particular implementation of the language.
Casting away shared is correct iff the data is owned by at most one thread after the cast.

On Wed, 17 Aug 2011 11:05:56 -0400, jdrewsen <jdrewsen@nospam.com> wrote:

> Den 17-08-2011 15:51, Steven Schveighoffer skrev:
>> On Wed, 17 Aug 2011 05:43:00 -0400, Jonas Drewsen <jdrewsen@nospam.com>
>> wrote:
>>
>>> On 17/08/11 00.21, Jonathan M Davis wrote:
>>>> On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
>>>>> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen<jdrewsen@nospam.com>
>>>>> wrote:
>>>>>> Den 16-08-2011 18:55, Martin Nowak skrev:
>>>>>>> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha<dsimcha@yahoo.com> wrote:
>>>>>>>> On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> This is a review request for the curl wrapper. Please read the
>>>>>>>>> "known
>>>>>>>>> issues" in the top of the source file and if possible suggest a
>>>>>>>>> solution.
>>>>>>>>>
>>>>>>>>> We also need somebody for running the review process. Anyone?
>>>>>>>>>
>>>>>>>>> Code:
>>>>>>>>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>>>>>>>>> Docs:
>>>>>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>>>>>>>
>>>>>>>>> Demolish!
>>>>>>>>>
>>>>>>>>> /Jonas
>>>>>>>>
>>>>>>>> From a quick look, this looks very well thought out. I'll review it
>>>>>>>> more thoroughly when I have more time. A few questions/comments
>>>>>>>> from a
>>>>>>>> quick look at the docs:
>>>>>>>>
>>>>>>>> Does the async stuff use D threads, or does Curl have its own async
>>>>>>>> API?
>>>>>>>>
>>>>>>>> In your examples for postData, you have onReceive a ubyte[] and
>>>>>>>> write
>>>>>>>> it out to console. Did you mean to cast this to some kind of string?
>>>>>>>>
>>>>>>>> For onReceive, what's the purpose of the return value?
>>>>>>>>
>>>>>>>> If/when this module makes it into Phobos, are we going to start
>>>>>>>> including a libcurl binary with DMD distributions so that std.curl
>>>>>>>> feels truly **standard** and requires zero extra configuration?
>>>>>>>
>>>>>>> I was also wondering about the async handling. In the long-term
>>>>>>> I'd like
>>>>>>> to see a bigger picture for async handling in phobos (offering
>>>>>>> some kind
>>>>>>> of futures, maybe event-loops etc.).
>>>>>>> Though this is not a requirement for the curl wrapper now.
>>>>>>> std.parallelism also has some kind of this stuff and file reading
>>>>>>> would
>>>>>>> benefit from it too.
>>>>>>
>>>>>> This has been discussed before and I also think this is very
>>>>>> important.
>>>>>> But before that I think some kind of package management should be
>>>>>> prioritized (A DIP11 implementaion or a more traditional solution).
>>>>>>
>>>>>>> One thing I spotted at a quick glance, sending to be filled
>>>>>>> buffers to
>>>>>>> another thread should not be done by casting to shared not immutable.
>>>>>>
>>>>>> I'm not entirely sure what you mean. There is no use of shared buffers
>>>>>> in the wrapper. I do cast the buffer between mutable/immutable because
>>>>>> only immutable or by value data can be passed using std.concurrency.
>>>>>> Since the buffers are only used by the thread that currently has the
>>>>>> buffer this is safe. I've previously asked for a non-cast solution
>>>>>> (ie.
>>>>>> some kind of move between threads semantic for std.concurrency) but
>>>>>> was
>>>>>> advised that this was the way to do it.
>>>>>>
>>>>>>> martin
>>>>>
>>>>> Pardon the typo. What I meant is that AFAIK casting from immutable to
>>>>> mutable has undefined behavior.
>>>>> The intended method for sending a uint[] buffer to another thread is to
>>>>> cast that
>>>>> buffer to shared (cast(shared(uint[])) and casting away the shared
>>>>> on the
>>>>> receiving side.
>>>>> It is allowed to send shared data using std.concurrency.
>>>>
>>>> Casting away immutability and then altering data is undefined. Actually
>>>> casting it away is defined. So, if you have data in one thread that
>>>> you know
>>>> is unique, you can cast it to immutable (or
>>>> std.exception.assumeUnique to do
>>>> it) and then send it to another thread. On that thread, you can then
>>>> cast it
>>>> to mutable and alter it.
>>>>
>>>> However, you're circumventing the type system when you do this. So,
>>>> you have
>>>> to be very careful. You're throwing away the guarantees that the
>>>> compiler
>>>> makes with regards to const and immutable. It _is_ guaranteed to work
>>>> though.
>>>> And I'm not sure that there's really any difference between casting
>>>> to shared
>>>> and back and casting to immutable and back. In both cases, you're
>>>> circumventing the type system. The main difference would be that if you
>>>> screwed up with immutable and cast away immutable on something that
>>>> really was
>>>> immutable rather than something that you cast to immutable just to
>>>> send it to
>>>> another thread, then you could a segfault when you tried to alter it,
>>>> since it
>>>> could be in ROM.
>>>>
>>>> - Jonathan M Davis
>>>
>>> Yeah I know you have to be careful when doing these kind of things. I
>>> ran into the problem of sending buffers between threads (using
>>> std.concurrency) so that they could be reused. There isn't any "move
>>> ownership" support in place so Andrei suggested I could do it by
>>> casting immutable.
>>>
>>> If anyone knows of a cleaner way to do this please tell.
>>
>> casting to shared and back. Passing shared data should be supported by
>> std.concurrency, and casting away shared is defined as long as you know
>> only one thread owns the data after casting.
>>
>> -Steve
>
> Why is this cleaner than casting to immutable and back?

Once it's immutable, it can never be mutable again.  Casting to immutable is a one-way street.  Yes, you can cast to mutable, but you still can't change the data unless you want undefined behavior.

Shared is not like that, an item can be thread-local, then shared, then thread local again, all the time being mutable.  It also reflects better what the process is (I'm sharing this data with another thread, then that thread is taking ownership).  There's still the possibility to screw up, but at least you are not in undefined territory in the correctly-implemented case.

-Steve

On Wednesday, August 17, 2011 11:30:06 Steven Schveighoffer wrote:
> On Wed, 17 Aug 2011 11:05:56 -0400, jdrewsen <jdrewsen@nospam.com> wrote:
> > Den 17-08-2011 15:51, Steven Schveighoffer skrev:
> >> On Wed, 17 Aug 2011 05:43:00 -0400, Jonas Drewsen <jdrewsen@nospam.com>
> >> 
> >> wrote:
> >>> On 17/08/11 00.21, Jonathan M Davis wrote:
> >>>> On Tuesday, August 16, 2011 12:32 Martin Nowak wrote:
> >>>>> On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen<jdrewsen@nospam.com>
> >>>>> 
> >>>>> wrote:
> >>>>>> Den 16-08-2011 18:55, Martin Nowak skrev:
> >>>>>>> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha<dsimcha@yahoo.com>
> >>>>>>> 
> >>>>>>> wrote:
> >>>>>>>> On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
> >>>>>>>>> Hi all,
> >>>>>>>>> 
> >>>>>>>>> This is a review request for the curl wrapper. Please
> >>>>>>>>> read the
> >>>>>>>>> "known
> >>>>>>>>> issues" in the top of the source file and if possible
> >>>>>>>>> suggest a
> >>>>>>>>> solution.
> >>>>>>>>> 
> >>>>>>>>> We also need somebody for running the review process. Anyone?
> >>>>>>>>> 
> >>>>>>>>> Code:
> >>>>>>>>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl
> >>>>>>>>> .d
> >>>>>>>>> Docs:
> >>>>>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
> >>>>>>>>> 
> >>>>>>>>> Demolish!
> >>>>>>>>> 
> >>>>>>>>> /Jonas
> >>>>>>>> 
> >>>>>>>> From a quick look, this looks very well thought out. I'll
> >>>>>>>> review
> >>>>>>>> it
> >>>>>>>> more thoroughly when I have more time. A few
> >>>>>>>> questions/comments
> >>>>>>>> from a
> >>>>>>>> quick look at the docs:
> >>>>>>>> 
> >>>>>>>> Does the async stuff use D threads, or does Curl have its
> >>>>>>>> own
> >>>>>>>> async
> >>>>>>>> API?
> >>>>>>>> 
> >>>>>>>> In your examples for postData, you have onReceive a
> >>>>>>>> ubyte[] and
> >>>>>>>> write
> >>>>>>>> it out to console. Did you mean to cast this to some kind
> >>>>>>>> of
> >>>>>>>> string?
> >>>>>>>> 
> >>>>>>>> For onReceive, what's the purpose of the return value?
> >>>>>>>> 
> >>>>>>>> If/when this module makes it into Phobos, are we going to
> >>>>>>>> start
> >>>>>>>> including a libcurl binary with DMD distributions so that
> >>>>>>>> std.curl
> >>>>>>>> feels truly **standard** and requires zero extra
> >>>>>>>> configuration?
> >>>>>>> 
> >>>>>>> I was also wondering about the async handling. In the
> >>>>>>> long-term
> >>>>>>> I'd like
> >>>>>>> to see a bigger picture for async handling in phobos
> >>>>>>> (offering
> >>>>>>> some kind
> >>>>>>> of futures, maybe event-loops etc.).
> >>>>>>> Though this is not a requirement for the curl wrapper now.
> >>>>>>> std.parallelism also has some kind of this stuff and file
> >>>>>>> reading
> >>>>>>> would
> >>>>>>> benefit from it too.
> >>>>>> 
> >>>>>> This has been discussed before and I also think this is very
> >>>>>> important.
> >>>>>> But before that I think some kind of package management should
> >>>>>> be
> >>>>>> prioritized (A DIP11 implementaion or a more traditional
> >>>>>> solution).
> >>>>>> 
> >>>>>>> One thing I spotted at a quick glance, sending to be filled
> >>>>>>> buffers to
> >>>>>>> another thread should not be done by casting to shared not
> >>>>>>> immutable.
> >>>>>> 
> >>>>>> I'm not entirely sure what you mean. There is no use of shared
> >>>>>> buffers
> >>>>>> in the wrapper. I do cast the buffer between mutable/immutable
> >>>>>> because
> >>>>>> only immutable or by value data can be passed using
> >>>>>> std.concurrency.
> >>>>>> Since the buffers are only used by the thread that currently
> >>>>>> has the
> >>>>>> buffer this is safe. I've previously asked for a non-cast
> >>>>>> solution
> >>>>>> (ie.
> >>>>>> some kind of move between threads semantic for
> >>>>>> std.concurrency) but
> >>>>>> was
> >>>>>> advised that this was the way to do it.
> >>>>>> 
> >>>>>>> martin
> >>>>> 
> >>>>> Pardon the typo. What I meant is that AFAIK casting from
> >>>>> immutable to
> >>>>> mutable has undefined behavior.
> >>>>> The intended method for sending a uint[] buffer to another
> >>>>> thread is
> >>>>> to
> >>>>> cast that
> >>>>> buffer to shared (cast(shared(uint[])) and casting away the
> >>>>> shared
> >>>>> on the
> >>>>> receiving side.
> >>>>> It is allowed to send shared data using std.concurrency.
> >>>> 
> >>>> Casting away immutability and then altering data is undefined.
> >>>> Actually
> >>>> casting it away is defined. So, if you have data in one thread
> >>>> that
> >>>> you know
> >>>> is unique, you can cast it to immutable (or
> >>>> std.exception.assumeUnique to do
> >>>> it) and then send it to another thread. On that thread, you can
> >>>> then
> >>>> cast it
> >>>> to mutable and alter it.
> >>>> 
> >>>> However, you're circumventing the type system when you do this.
> >>>> So,
> >>>> you have
> >>>> to be very careful. You're throwing away the guarantees that the
> >>>> compiler
> >>>> makes with regards to const and immutable. It _is_ guaranteed to
> >>>> work
> >>>> though.
> >>>> And I'm not sure that there's really any difference between
> >>>> casting
> >>>> to shared
> >>>> and back and casting to immutable and back. In both cases, you're
> >>>> circumventing the type system. The main difference would be that
> >>>> if
> >>>> you
> >>>> screwed up with immutable and cast away immutable on something
> >>>> that
> >>>> really was
> >>>> immutable rather than something that you cast to immutable just to
> >>>> send it to
> >>>> another thread, then you could a segfault when you tried to alter
> >>>> it,
> >>>> since it
> >>>> could be in ROM.
> >>>> 
> >>>> - Jonathan M Davis
> >>> 
> >>> Yeah I know you have to be careful when doing these kind of things.
> >>> I
> >>> ran into the problem of sending buffers between threads (using
> >>> std.concurrency) so that they could be reused. There isn't any "move
> >>> ownership" support in place so Andrei suggested I could do it by
> >>> casting immutable.
> >>> 
> >>> If anyone knows of a cleaner way to do this please tell.
> >> 
> >> casting to shared and back. Passing shared data should be supported by
> >> std.concurrency, and casting away shared is defined as long as you
> >> know
> >> only one thread owns the data after casting.
> >> 
> >> -Steve
> > 
> > Why is this cleaner than casting to immutable and back?
> 
> Once it's immutable, it can never be mutable again.  Casting to immutable is a one-way street.  Yes, you can cast to mutable, but you still can't change the data unless you want undefined behavior.
> 
> Shared is not like that, an item can be thread-local, then shared, then thread local again, all the time being mutable.  It also reflects better what the process is (I'm sharing this data with another thread, then that thread is taking ownership).  There's still the possibility to screw up, but at least you are not in undefined territory in the correctly-implemented case.

Are you sure? As I understand it, there's no real difference between casting to immutable and back and casting to shared and back. Both circumvent the type system. In the one case, the type system guarantees that the data can't be altered, and you're breaking that guarantee, because you know that it _can_ be, since you created the data and know that it's actually mutable. In the other case, the type system guarantees that the data is thread-local and therefore thread-safe, and you're breaking that guarantee by casting it to shared. On the other end, you're then casting it back, since you know that the data isn't actually shared. In both cases, you're circumventing the compiler's guarantees. In both cases, you've claimed that it's thread for th second thread to use the data, when if you screwed up and left references to it in the first thread, then it isn't. I don't really see the difference.

- Jonathan M Davis