www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - CURL review request

reply Jonas Drewsen <jdrewsen nospam.com> writes:
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
Aug 16 2011
next sibling parent Jacob Carlborg <doob me.com> writes:
On 2011-08-16 13:48, 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


Is it just me or is it very annoying that the jump-to section in the 
documentation shows non-top declarations, e.g. methods, enum members, 
struct functions and so on.

I know this is not specific to the curl documentation.

-- 
/Jacob Carlborg
Aug 16 2011
prev sibling next sibling parent reply bearophile <bearophileHUGS lycos.com> writes:
Jonas Drewsen:


 Demolish!


I'd like to try this module. Do you have a compiled curl lib for Windows?

Bye,
bearophile
Aug 16 2011
parent "Lars T. Kyllingstad" <public kyllingen.NOSPAMnet> writes:
On Tue, 16 Aug 2011 08:53:12 -0400, bearophile wrote:


 Jonas Drewsen:
 

 Demolish!


 
 I'd like to try this module. Do you have a compiled curl lib for
 Windows?


http://curl.haxx.se/download.html

-Lars
Aug 16 2011
prev sibling next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-08-16 13:48, 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


The ddoc issue: http://d.puremagic.com/issues/show_bug.cgi?id=648
I would very much like to have this fixed.

-- 
/Jacob Carlborg
Aug 16 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 14:57, Jacob Carlborg skrev:

 On 2011-08-16 13:48, 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


 The ddoc issue: http://d.puremagic.com/issues/show_bug.cgi?id=648
 I would very much like to have this fixed.


Ahh... thank you for the pointer.

That is only one part of the ddoc issues. The other one is that the 
Protocol mixin (which is mixed into the Http/Ftp/Smtp classes) does not 
generate documentation in the Http/Ftp/Smtp classes. I guess that is 
another bug/feature.
Aug 16 2011
prev sibling next sibling parent reply dsimcha <dsimcha yahoo.com> writes:
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?
Aug 16 2011
next sibling parent reply "Martin Nowak" <dawg dawgfoto.de> writes:
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.

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.

martin
Aug 16 2011
parent reply jdrewsen <jdrewsen nospam.com> writes:
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
Aug 16 2011
next sibling parent "Martin Nowak" <dawg dawgfoto.de> writes:
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.

martin
Aug 16 2011
prev sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
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
Aug 16 2011
parent reply Jonas Drewsen <jdrewsen nospam.com> writes:
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
Aug 17 2011
parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
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
Aug 17 2011
parent reply jdrewsen <jdrewsen nospam.com> writes:
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
Aug 17 2011
next sibling parent Timon Gehr <timon.gehr gmx.ch> writes:
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.
Aug 17 2011
prev sibling parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
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
Aug 17 2011
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
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
Aug 17 2011
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


No. As soon as the data is typed as immutable anywhere it cannot be 
changed anymore. You only break guarantees if you actually try to change 
the data (otherwise std.typecons.assumeUnique would perform its job 
outside defined behavior btw)


 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


You don't break any guarantees when casting away shared if you know that 
the data is actually not shared anymore.
(Of course, if it is actually still shared between multiple threads, 
this is about as bad as altering data which is typed as immutable 
somewhere.)


You don't break any guarantees if you don't actually break them. The 
casts are just there because the compiler is unable to verify that you 
don't.

Therefore casting to immutable and back and then changing data is bad, 
but casting data to shared, transferring ownership to another single 
thread and then casting back to unshared is good.
Aug 17 2011
next sibling parent "Martin Nowak" <dawg dawgfoto.de> writes:
On Wed, 17 Aug 2011 18:21:00 +0200, Timon Gehr <timon.gehr gmx.ch> wrote:


 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be  
 changed anymore. You only break guarantees if you actually try to change  
 the data (otherwise std.typecons.assumeUnique would perform its job  
 outside defined behavior btw)


 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


 You don't break any guarantees when casting away shared if you know that  
 the data is actually not shared anymore.
 (Of course, if it is actually still shared between multiple threads,  
 this is about as bad as altering data which is typed as immutable  
 somewhere.)


 You don't break any guarantees if you don't actually break them. The  
 casts are just there because the compiler is unable to verify that you  
 don't.

 Therefore casting to immutable and back and then changing data is bad,  
 but casting data to shared, transferring ownership to another single  
 thread and then casting back to unshared is good.


Not wanting to drift too far off topic I'll add one last point.
given:
immutable(int[]) data = assumeUnique(myints);
send(cast(int[])data);
writeln(data[0]);

A compiler implementation could deduce data won't change between  
initialization and the write.
Thus performing optimizations that would break the code.
I think that and being able to store ctfe data in read only sections are  
the reasons for the undefined behavior.
Removing Immutable With A Cast =>  
http://www.digitalmars.com/d/2.0/const3.html

martin
Aug 17 2011
prev sibling next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, August 17, 2011 13:31 Martin Nowak wrote:

 Not wanting to drift too far off topic I'll add one last point.
 given:
 immutable(int[]) data = assumeUnique(myints);
 send(cast(int[])data);
 writeln(data[0]);
 
 A compiler implementation could deduce data won't change between
 initialization and the write.
 Thus performing optimizations that would break the code.
 I think that and being able to store ctfe data in read only sections are
 the reasons for the undefined behavior.
 Removing Immutable With A Cast =>
 http://www.digitalmars.com/d/2.0/const3.html


Yes, because you actually kept the data on the original thread. The _only_ way 
that passing via send by casting to and from share or to and from immutable is 
going to work properly if you _do not keep the data on the original thread_. 
By casting to either shared or immutable, you're breaking the guarantees that 
the compiler makes and expects. So, doing something like using that data again 
on the original thread is broken regardless. The _only_ time that this way of 
passing mutable data via send is okay is when you're actually passing full 
ownership of the data across and never touching it on the original thread 
again (unless you pass it back across the same way, handing over ownership 
again). Your code is broken regardless of whether you're using immutable or 
shared. True, immutable might be more thoroughly optimized than shared is 
therefore more likely to break, but the code is broken regardless.

- Jonathan M Davis
Aug 17 2011
parent Timon Gehr <timon.gehr gmx.ch> writes:
On 08/17/2011 11:09 PM, Jonathan M Davis wrote:

 On Wednesday, August 17, 2011 13:31 Martin Nowak wrote:

 Not wanting to drift too far off topic I'll add one last point.
 given:
 immutable(int[]) data = assumeUnique(myints);
 send(cast(int[])data);
 writeln(data[0]);

 A compiler implementation could deduce data won't change between
 initialization and the write.
 Thus performing optimizations that would break the code.
 I think that and being able to store ctfe data in read only sections are
 the reasons for the undefined behavior.
 Removing Immutable With A Cast =>
 http://www.digitalmars.com/d/2.0/const3.html


 Yes, because you actually kept the data on the original thread. The _only_ way
 that passing via send by casting to and from share or to and from immutable is
 going to work properly if you _do not keep the data on the original thread_.
 By casting to either shared or immutable, you're breaking the guarantees that
 the compiler makes and expects. So, doing something like using that data again
 on the original thread is broken regardless. The _only_ time that this way of
 passing mutable data via send is okay is when you're actually passing full
 ownership of the data across and never touching it on the original thread
 again (unless you pass it back across the same way, handing over ownership
 again). Your code is broken regardless of whether you're using immutable or
 shared. True, immutable might be more thoroughly optimized than shared is
 therefore more likely to break, but the code is broken regardless.

 - Jonathan M Davis


No, casting shared and back carefully does not break the code, as I 
pointed out in my previous post. Casting immutable and back does.

You don't break the guarantees the compiler expects and makes with 
transferring ownership by cast to shared and back.


int         // this variable is visible to one thread only
shared(int) // this variable might be visible to multiple threads


If you *know* that your shared variable is visible by only one thread, 
you can safely cast it to unshared, without breaking any guarantees.

You can always safely cast to shared, because shared gives less guarantees.

Doing it by casting to immutable and back, on the other hand, is wrong.

int            // this variable may change its value
immutable(int) // this variable may not

If you *know* that your mutable variable is never going to change, you 
can safely cast it to immutable, without breaking any guarantees.

If you *know* that your immutable variable is never going to change 
after the cast, you can safely cast it to mutable, without breaking any 
guarantees.

But in this case it is probably going to change, so it does not work 
with immutable. shared and immutable are two very different things.
Aug 17 2011
prev sibling parent reply jdrewsen <jdrewsen nospam.com> writes:
Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to change
 the data (otherwise std.typecons.assumeUnique would perform its job
 outside defined behavior btw)


I'm thinking down the same lines as Jonathan. Is the behavior for 
immutable casts that you describe specified in the language reference 
somewhere?

I have no problem with using shared casts instead of immutable - I just 
want make sure it is really needed.



 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


 You don't break any guarantees when casting away shared if you know that
 the data is actually not shared anymore.
 (Of course, if it is actually still shared between multiple threads,
 this is about as bad as altering data which is typed as immutable
 somewhere.)


 You don't break any guarantees if you don't actually break them. The
 casts are just there because the compiler is unable to verify that you
 don't.

 Therefore casting to immutable and back and then changing data is bad,
 but casting data to shared, transferring ownership to another single
 thread and then casting back to unshared is good.
Aug 18 2011
next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 
 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to change
 the data (otherwise std.typecons.assumeUnique would perform its job
 outside defined behavior btw)


 
 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?
 
 I have no problem with using shared casts instead of immutable - I just
 want make sure it is really needed.


The behavior of casting a way const or immutable on a value and then mutating 
it is undefined by the language, because you're breaking the language's 
guarantees and what happens depends entirely on whether the actual object was 
actually immutable. However, in the case of casting to immutable and then 
casting back, you _know_ that the object is mutable, so there's no problem. 
You're just circumventing the type system which throws away the guarantees 
that it gives you about immutability, which could screw up optimizations if 
you had actually did more than just pass the variable around. But that's just 
not happening here.

As for casting to and from shared and mutating the object, I don't see how it 
is any more defined than casting to and from immutable and then mutating the 
object is. In both cases, you circumvented the type system, which breaks the 
compiler's guarantees and risks bugs if you actually do more than just pass 
the variable around before casting it back to being thread-local and mutable.

- Jonathan M Davis
Aug 18 2011
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 08/19/2011 12:35 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to change
 the data (otherwise std.typecons.assumeUnique would perform its job
 outside defined behavior btw)


 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?

 I have no problem with using shared casts instead of immutable - I just
 want make sure it is really needed.


 The behavior of casting a way const or immutable on a value and then mutating
 it is undefined by the language, because you're breaking the language's
 guarantees and what happens depends entirely on whether the actual object was
 actually immutable. However, in the case of casting to immutable and then
 casting back, you _know_ that the object is mutable, so there's no problem.
 You're just circumventing the type system which throws away the guarantees
 that it gives you about immutability, which could screw up optimizations if
 you had actually did more than just pass the variable around. But that's just
 not happening here.

 As for casting to and from shared and mutating the object, I don't see how it
 is any more defined than casting to and from immutable and then mutating the
 object is. In both cases, you circumvented the type system, which breaks the
 compiler's guarantees and risks bugs if you actually do more than just pass
 the variable around before casting it back to being thread-local and mutable.

 - Jonathan M Davis


As long as the data is not being shared between multiple threads after 
it's sharedness has been cast away, you are well in defined area, 
because you are NOT breaking anything.

The crucial difference between immutable and shared is, that something 
that is immutable will always be immutable, but being shared or not may 
change dynamically.

Casting to immutable is a one-way-street, while casting to shared is not.
Aug 18 2011
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Thursday, August 18, 2011 16:00 Timon Gehr wrote:

 On 08/19/2011 12:35 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 
 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to
 change the data (otherwise std.typecons.assumeUnique would perform its
 job outside defined behavior btw)


 
 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?
 
 I have no problem with using shared casts instead of immutable - I just
 want make sure it is really needed.


 
 The behavior of casting a way const or immutable on a value and then
 mutating it is undefined by the language, because you're breaking the
 language's guarantees and what happens depends entirely on whether the
 actual object was actually immutable. However, in the case of casting to
 immutable and then casting back, you _know_ that the object is mutable,
 so there's no problem. You're just circumventing the type system which
 throws away the guarantees that it gives you about immutability, which
 could screw up optimizations if you had actually did more than just pass
 the variable around. But that's just not happening here.
 
 As for casting to and from shared and mutating the object, I don't see
 how it is any more defined than casting to and from immutable and then
 mutating the object is. In both cases, you circumvented the type system,
 which breaks the compiler's guarantees and risks bugs if you actually do
 more than just pass the variable around before casting it back to being
 thread-local and mutable.
 
 - Jonathan M Davis


 
 As long as the data is not being shared between multiple threads after
 it's sharedness has been cast away, you are well in defined area,
 because you are NOT breaking anything.


The _only_ reason that you're not breaking anything is because you are being 
careful and making sure that the data is not actually shared between threads. 
You're _still_ circumventing the type system and breaking the guarantee that a 
non-shared variable is not shared between threads. _You_ are the one 
guaranteeing that the variable is only on one thread, not the compiler. And 
when you cast away immutable, _you_ are the one guaranteeing that immutable 
data is not being mutated, not the compiler. And in this case, you can make 
that guarantee in exactly the same way that you can guarantee that the 
variable which was cast to shared and back to be passed across threads isn't 
actually shared between threads once it has been passed.


 The crucial difference between immutable and shared is, that something
 that is immutable will always be immutable, but being shared or not may
 change dynamically.
 
 Casting to immutable is a one-way-street, while casting to shared is not.


Casting to immutable does not make the data magically immutable. It makes it 
so that the compiler treats it as immutable. Casting from immutable makes it 
so that the compiler treats it as mutable. It does not alter whether the data 
is actually immutable. Casting away immutable and altering data is undefined, 
because it depends entirely on whether the data is actually immutable or not. 
If it isn't, and you don't have any other references to the data (or none of 
the other references are immutable), then you don't break _anything_ by 
mutating the variable after having cast away immutable.

With both shared and immutable, casting it away is circumnventing the type 
system. In both cases, _you_ must make sure that you code in a way which does 
not violate the guarantees that the compiler normally makes about those types. 
If you do violate those guarantees (e.g. by sharing non-shared data across 
threads or by mutating data which really was immutable), then you have a bug, 
and what happens is dependent on the compiler implementation and on your code. 
But if you don't violate those guarantees, then your program is fine. It's the 
same for both shared and immutable. It's just that the bugs that you're likely 
to get when you violate those guarantees are likely to be quite different.

- Jonathan M Davis
Aug 18 2011
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 08/19/2011 01:50 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 16:00 Timon Gehr wrote:

 On 08/19/2011 12:35 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to
 change the data (otherwise std.typecons.assumeUnique would perform its
 job outside defined behavior btw)


 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?

 I have no problem with using shared casts instead of immutable - I just
 want make sure it is really needed.


 The behavior of casting a way const or immutable on a value and then
 mutating it is undefined by the language, because you're breaking the
 language's guarantees and what happens depends entirely on whether the
 actual object was actually immutable. However, in the case of casting to
 immutable and then casting back, you _know_ that the object is mutable,
 so there's no problem. You're just circumventing the type system which
 throws away the guarantees that it gives you about immutability, which
 could screw up optimizations if you had actually did more than just pass
 the variable around. But that's just not happening here.

 As for casting to and from shared and mutating the object, I don't see
 how it is any more defined than casting to and from immutable and then
 mutating the object is. In both cases, you circumvented the type system,
 which breaks the compiler's guarantees and risks bugs if you actually do
 more than just pass the variable around before casting it back to being
 thread-local and mutable.

 - Jonathan M Davis


 As long as the data is not being shared between multiple threads after
 it's sharedness has been cast away, you are well in defined area,
 because you are NOT breaking anything.


 The _only_ reason that you're not breaking anything is because you are being
 careful and making sure that the data is not actually shared between threads.
 You're _still_ circumventing the type system and breaking the guarantee that a
 non-shared variable is not shared between threads. _You_ are the one
 guaranteeing that the variable is only on one thread, not the compiler. And
 when you cast away immutable, _you_ are the one guaranteeing that immutable
 data is not being mutated, not the compiler. And in this case, you can make
 that guarantee in exactly the same way that you can guarantee that the
 variable which was cast to shared and back to be passed across threads isn't
 actually shared between threads once it has been passed.


 The crucial difference between immutable and shared is, that something
 that is immutable will always be immutable, but being shared or not may
 change dynamically.

 Casting to immutable is a one-way-street, while casting to shared is not.


 Casting to immutable does not make the data magically immutable.


Yes it does. That is quite exactly the definition of it.


 It makes it
 so that the compiler treats it as immutable.


The compiler is irrelevant, the fact that one compiler generates 
assembly that behaves as you'd like it to behave does not mean the code 
is valid.


 Casting from immutable makes it
 so that the compiler treats it as mutable. It does not alter whether the data
 is actually immutable.


'Actually immutable' means that the variable is never changed. The 
language spec says that if an immutable variable is not 'actually 
immutable', your program is in error, even if immutability has been 
explicitly cast away.


 Casting away immutable and altering data is undefined,
 because it depends entirely on whether the data is actually immutable or not.


Just to make sure we agree on that: knowing why it is undefined does not 
imply there are cases where it is actually defined.


 If it isn't, and you don't have any other references to the data (or none of
 the other references are immutable), then you don't break _anything_ by
 mutating the variable after having cast away immutable.


If you believe the spec, you do break validity of your code even if the 
variable is not visible from another place after the cast. There is 
currently afaik no reason why such code would *have* to be broken, but 
that is what the spec says, because it categorically bans changing 
variables that were cast from immutable.


 With both shared and immutable, casting it away is circumnventing the type
 system. In both cases, _you_ must make sure that you code in a way which does
 not violate the guarantees that the compiler normally makes about those types.
 If you do violate those guarantees (e.g. by sharing non-shared data across
 threads or by mutating data which really was immutable), then you have a bug,
 and what happens is dependent on the compiler implementation and on your code.


Agreed, and this means what happens is undefined and such code is invalid.


 But if you don't violate those guarantees, then your program is fine. It's the
 same for both shared and immutable. It's just that the bugs that you're likely
 to get when you violate those guarantees are likely to be quite different.

 - Jonathan M Davis


Yes, I agree. And in this specific case you do violate the language 
guarantees about immutable variables when you change the variable after 
casting it back to mutable. Even if it was typed mutable sometime 
before, and even if it is the sole reference in the whole program.
Therefore the behavior of the current implementation is undefined under 
the current spec, and changing the transfer protocol to use shared 
instead of immutable would fix this. (while generating near-identical 
assembly output with the current DMD)
Aug 19 2011
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Friday, August 19, 2011 04:58 Timon Gehr wrote:

 On 08/19/2011 01:50 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 16:00 Timon Gehr wrote:

 On 08/19/2011 12:35 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 
 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to
 change the data (otherwise std.typecons.assumeUnique would perform
 its job outside defined behavior btw)


 
 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?
 
 I have no problem with using shared casts instead of immutable - I
 just want make sure it is really needed.


 
 The behavior of casting a way const or immutable on a value and then
 mutating it is undefined by the language, because you're breaking the
 language's guarantees and what happens depends entirely on whether the
 actual object was actually immutable. However, in the case of casting
 to immutable and then casting back, you _know_ that the object is
 mutable, so there's no problem. You're just circumventing the type
 system which throws away the guarantees that it gives you about
 immutability, which could screw up optimizations if you had actually
 did more than just pass the variable around. But that's just not
 happening here.
 
 As for casting to and from shared and mutating the object, I don't see
 how it is any more defined than casting to and from immutable and then
 mutating the object is. In both cases, you circumvented the type
 system, which breaks the compiler's guarantees and risks bugs if you
 actually do more than just pass the variable around before casting it
 back to being thread-local and mutable.
 
 - Jonathan M Davis


 
 As long as the data is not being shared between multiple threads after
 it's sharedness has been cast away, you are well in defined area,
 because you are NOT breaking anything.


 
 The _only_ reason that you're not breaking anything is because you are
 being careful and making sure that the data is not actually shared
 between threads. You're _still_ circumventing the type system and
 breaking the guarantee that a non-shared variable is not shared between
 threads. _You_ are the one guaranteeing that the variable is only on one
 thread, not the compiler. And when you cast away immutable, _you_ are
 the one guaranteeing that immutable data is not being mutated, not the
 compiler. And in this case, you can make that guarantee in exactly the
 same way that you can guarantee that the variable which was cast to
 shared and back to be passed across threads isn't actually shared
 between threads once it has been passed.
 

 The crucial difference between immutable and shared is, that something
 that is immutable will always be immutable, but being shared or not may
 change dynamically.
 
 Casting to immutable is a one-way-street, while casting to shared is
 not.


 
 Casting to immutable does not make the data magically immutable.


 
 Yes it does. That is quite exactly the definition of it.
 

 It makes it
 so that the compiler treats it as immutable.


 
 The compiler is irrelevant, the fact that one compiler generates
 assembly that behaves as you'd like it to behave does not mean the code
 is valid.
 

 Casting from immutable makes it
 so that the compiler treats it as mutable. It does not alter whether the
 data is actually immutable.


 
 'Actually immutable' means that the variable is never changed. The
 language spec says that if an immutable variable is not 'actually
 immutable', your program is in error, even if immutability has been
 explicitly cast away.
 

 Casting away immutable and altering data is undefined,
 because it depends entirely on whether the data is actually immutable or
 not.


 
 Just to make sure we agree on that: knowing why it is undefined does not
 imply there are cases where it is actually defined.
 

 If it isn't, and you don't have any other references to the data (or none
 of the other references are immutable), then you don't break _anything_
 by mutating the variable after having cast away immutable.


 
 If you believe the spec, you do break validity of your code even if the
 variable is not visible from another place after the cast. There is
 currently afaik no reason why such code would *have* to be broken, but
 that is what the spec says, because it categorically bans changing
 variables that were cast from immutable.
 

 With both shared and immutable, casting it away is circumnventing the
 type system. In both cases, _you_ must make sure that you code in a way
 which does not violate the guarantees that the compiler normally makes
 about those types. If you do violate those guarantees (e.g. by sharing
 non-shared data across threads or by mutating data which really was
 immutable), then you have a bug, and what happens is dependent on the
 compiler implementation and on your code.


 
 Agreed, and this means what happens is undefined and such code is invalid.
 

 But if you don't violate those guarantees, then your program is fine.
 It's the same for both shared and immutable. It's just that the bugs
 that you're likely to get when you violate those guarantees are likely
 to be quite different.
 
 - Jonathan M Davis


 
 Yes, I agree. And in this specific case you do violate the language
 guarantees about immutable variables when you change the variable after
 casting it back to mutable. Even if it was typed mutable sometime
 before, and even if it is the sole reference in the whole program.
 Therefore the behavior of the current implementation is undefined under
 the current spec, and changing the transfer protocol to use shared
 instead of immutable would fix this. (while generating near-identical
 assembly output with the current DMD)


What I completely disagree with you on is that casting something to shared, 
passing it to another thread, casting from shared, and then altering a 
variable is any more defined than casting to and from immutable and altering a 
variable is. In _both_ cases, you are circumventing the compiler, and in 
_both_ cases, _you_ must guarantee that what you're doing doesn't actually 
violate what the compiler is trying to guarantee.

- Jonathan M Davis
Aug 19 2011
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 08/19/2011 10:36 PM, Jonathan M Davis wrote:

 On Friday, August 19, 2011 04:58 Timon Gehr wrote:

 On 08/19/2011 01:50 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 16:00 Timon Gehr wrote:

 On 08/19/2011 12:35 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to
 change the data (otherwise std.typecons.assumeUnique would perform
 its job outside defined behavior btw)


 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?

 I have no problem with using shared casts instead of immutable - I
 just want make sure it is really needed.


 The behavior of casting a way const or immutable on a value and then
 mutating it is undefined by the language, because you're breaking the
 language's guarantees and what happens depends entirely on whether the
 actual object was actually immutable. However, in the case of casting
 to immutable and then casting back, you _know_ that the object is
 mutable, so there's no problem. You're just circumventing the type
 system which throws away the guarantees that it gives you about
 immutability, which could screw up optimizations if you had actually
 did more than just pass the variable around. But that's just not
 happening here.

 As for casting to and from shared and mutating the object, I don't see
 how it is any more defined than casting to and from immutable and then
 mutating the object is. In both cases, you circumvented the type
 system, which breaks the compiler's guarantees and risks bugs if you
 actually do more than just pass the variable around before casting it
 back to being thread-local and mutable.

 - Jonathan M Davis


 As long as the data is not being shared between multiple threads after
 it's sharedness has been cast away, you are well in defined area,
 because you are NOT breaking anything.


 The _only_ reason that you're not breaking anything is because you are
 being careful and making sure that the data is not actually shared
 between threads. You're _still_ circumventing the type system and
 breaking the guarantee that a non-shared variable is not shared between
 threads. _You_ are the one guaranteeing that the variable is only on one
 thread, not the compiler. And when you cast away immutable, _you_ are
 the one guaranteeing that immutable data is not being mutated, not the
 compiler. And in this case, you can make that guarantee in exactly the
 same way that you can guarantee that the variable which was cast to
 shared and back to be passed across threads isn't actually shared
 between threads once it has been passed.


 The crucial difference between immutable and shared is, that something
 that is immutable will always be immutable, but being shared or not may
 change dynamically.

 Casting to immutable is a one-way-street, while casting to shared is
 not.


 Casting to immutable does not make the data magically immutable.


 Yes it does. That is quite exactly the definition of it.


 It makes it
 so that the compiler treats it as immutable.


 The compiler is irrelevant, the fact that one compiler generates
 assembly that behaves as you'd like it to behave does not mean the code
 is valid.


 Casting from immutable makes it
 so that the compiler treats it as mutable. It does not alter whether the
 data is actually immutable.


 'Actually immutable' means that the variable is never changed. The
 language spec says that if an immutable variable is not 'actually
 immutable', your program is in error, even if immutability has been
 explicitly cast away.


 Casting away immutable and altering data is undefined,
 because it depends entirely on whether the data is actually immutable or
 not.


 Just to make sure we agree on that: knowing why it is undefined does not
 imply there are cases where it is actually defined.


 If it isn't, and you don't have any other references to the data (or none
 of the other references are immutable), then you don't break _anything_
 by mutating the variable after having cast away immutable.


 If you believe the spec, you do break validity of your code even if the
 variable is not visible from another place after the cast. There is
 currently afaik no reason why such code would *have* to be broken, but
 that is what the spec says, because it categorically bans changing
 variables that were cast from immutable.


 With both shared and immutable, casting it away is circumnventing the
 type system. In both cases, _you_ must make sure that you code in a way
 which does not violate the guarantees that the compiler normally makes
 about those types. If you do violate those guarantees (e.g. by sharing
 non-shared data across threads or by mutating data which really was
 immutable), then you have a bug, and what happens is dependent on the
 compiler implementation and on your code.


 Agreed, and this means what happens is undefined and such code is invalid.


 But if you don't violate those guarantees, then your program is fine.
 It's the same for both shared and immutable. It's just that the bugs
 that you're likely to get when you violate those guarantees are likely
 to be quite different.

 - Jonathan M Davis


 Yes, I agree. And in this specific case you do violate the language
 guarantees about immutable variables when you change the variable after
 casting it back to mutable. Even if it was typed mutable sometime
 before, and even if it is the sole reference in the whole program.
 Therefore the behavior of the current implementation is undefined under
 the current spec, and changing the transfer protocol to use shared
 instead of immutable would fix this. (while generating near-identical
 assembly output with the current DMD)


 What I completely disagree with you on is that casting something to shared,
 passing it to another thread, casting from shared, and then altering a
 variable is any more defined than casting to and from immutable and altering a
 variable is. In _both_ cases, you are circumventing the compiler, and in
 _both_ cases, _you_ must guarantee that what you're doing doesn't actually
 violate what the compiler is trying to guarantee.

 - Jonathan M Davis


As I said, in the case of immutable, changing the data is the problem:
It does actually violate the rules of the spec. It is explicitly 
disallowed to change any data that has been cast from immutable. (still, 
with DMD it probably works.)

shared has no such strong after-the-cast limitation (at least I haven't 
found a reason or a passage in the spec), the data should just not be 
visible by multiple threads afterwards. You may well change the variable 
after the cast.
Aug 19 2011
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Friday, August 19, 2011 15:27 Timon Gehr wrote:

 On 08/19/2011 10:36 PM, Jonathan M Davis wrote:

 On Friday, August 19, 2011 04:58 Timon Gehr wrote:

 On 08/19/2011 01:50 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 16:00 Timon Gehr wrote:

 On 08/19/2011 12:35 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 
 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to
 change the data (otherwise std.typecons.assumeUnique would perform
 its job outside defined behavior btw)


 
 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language
 reference somewhere?
 
 I have no problem with using shared casts instead of immutable - I
 just want make sure it is really needed.


 
 The behavior of casting a way const or immutable on a value and then
 mutating it is undefined by the language, because you're breaking the
 language's guarantees and what happens depends entirely on whether
 the actual object was actually immutable. However, in the case of
 casting to immutable and then casting back, you _know_ that the
 object is mutable, so there's no problem. You're just circumventing
 the type system which throws away the guarantees that it gives you
 about immutability, which could screw up optimizations if you had
 actually did more than just pass the variable around. But that's
 just not happening here.
 
 As for casting to and from shared and mutating the object, I don't
 see how it is any more defined than casting to and from immutable
 and then mutating the object is. In both cases, you circumvented the
 type system, which breaks the compiler's guarantees and risks bugs
 if you actually do more than just pass the variable around before
 casting it back to being thread-local and mutable.
 
 - Jonathan M Davis


 
 As long as the data is not being shared between multiple threads after
 it's sharedness has been cast away, you are well in defined area,
 because you are NOT breaking anything.


 
 The _only_ reason that you're not breaking anything is because you are
 being careful and making sure that the data is not actually shared
 between threads. You're _still_ circumventing the type system and
 breaking the guarantee that a non-shared variable is not shared between
 threads. _You_ are the one guaranteeing that the variable is only on
 one thread, not the compiler. And when you cast away immutable, _you_
 are the one guaranteeing that immutable data is not being mutated, not
 the compiler. And in this case, you can make that guarantee in exactly
 the same way that you can guarantee that the variable which was cast
 to shared and back to be passed across threads isn't actually shared
 between threads once it has been passed.
 

 The crucial difference between immutable and shared is, that something
 that is immutable will always be immutable, but being shared or not
 may change dynamically.
 
 Casting to immutable is a one-way-street, while casting to shared is
 not.


 
 Casting to immutable does not make the data magically immutable.


 
 Yes it does. That is quite exactly the definition of it.
 

 It makes it
 so that the compiler treats it as immutable.


 
 The compiler is irrelevant, the fact that one compiler generates
 assembly that behaves as you'd like it to behave does not mean the code
 is valid.
 

 Casting from immutable makes it
 so that the compiler treats it as mutable. It does not alter whether
 the data is actually immutable.


 
 'Actually immutable' means that the variable is never changed. The
 language spec says that if an immutable variable is not 'actually
 immutable', your program is in error, even if immutability has been
 explicitly cast away.
 

 Casting away immutable and altering data is undefined,
 because it depends entirely on whether the data is actually immutable
 or not.


 
 Just to make sure we agree on that: knowing why it is undefined does not
 imply there are cases where it is actually defined.
 

 If it isn't, and you don't have any other references to the data (or
 none of the other references are immutable), then you don't break
 _anything_ by mutating the variable after having cast away immutable.


 
 If you believe the spec, you do break validity of your code even if the
 variable is not visible from another place after the cast. There is
 currently afaik no reason why such code would *have* to be broken, but
 that is what the spec says, because it categorically bans changing
 variables that were cast from immutable.
 

 With both shared and immutable, casting it away is circumnventing the
 type system. In both cases, _you_ must make sure that you code in a way
 which does not violate the guarantees that the compiler normally makes
 about those types. If you do violate those guarantees (e.g. by sharing
 non-shared data across threads or by mutating data which really was
 immutable), then you have a bug, and what happens is dependent on the
 compiler implementation and on your code.


 
 Agreed, and this means what happens is undefined and such code is
 invalid.
 

 But if you don't violate those guarantees, then your program is fine.
 It's the same for both shared and immutable. It's just that the bugs
 that you're likely to get when you violate those guarantees are likely
 to be quite different.
 
 - Jonathan M Davis


 
 Yes, I agree. And in this specific case you do violate the language
 guarantees about immutable variables when you change the variable after
 casting it back to mutable. Even if it was typed mutable sometime
 before, and even if it is the sole reference in the whole program.
 Therefore the behavior of the current implementation is undefined under
 the current spec, and changing the transfer protocol to use shared
 instead of immutable would fix this. (while generating near-identical
 assembly output with the current DMD)


 
 What I completely disagree with you on is that casting something to
 shared, passing it to another thread, casting from shared, and then
 altering a variable is any more defined than casting to and from
 immutable and altering a variable is. In _both_ cases, you are
 circumventing the compiler, and in _both_ cases, _you_ must guarantee
 that what you're doing doesn't actually violate what the compiler is
 trying to guarantee.
 
 - Jonathan M Davis


 
 As I said, in the case of immutable, changing the data is the problem:
 It does actually violate the rules of the spec. It is explicitly
 disallowed to change any data that has been cast from immutable. (still,
 with DMD it probably works.)
 
 shared has no such strong after-the-cast limitation (at least I haven't
 found a reason or a passage in the spec), the data should just not be
 visible by multiple threads afterwards. You may well change the variable
 after the cast.


Casting away immutable on a variable and mutating it is _not_ disallowed by 
the spec. It is undefined as to what happens when you mutate the data after 
having cast away immutable. This is because it's completely dependent on what 
the actual state of the data was. If it's actually immutable (e.g. it's in 
ROM), then things are going to blow up on you. If it's not actually immutable, 
but there are other references to the data, then the mutation of the variable 
can cause bugs because the compiler makes assumptions based on the guarantees 
that immutable provides, and you broke those guarantees. So, as long as the 
variable was created as a mutable variable and there are no longer any 
immutable references to that data which could be relying on that data to not 
change, then there are _zero_ problems with casting away immutable and 
mutating a variable. It _is_ not disallowed to do that. It is merely 
_undefined_ as to what happens. If it were disallowed, then you couldn't do.

In the case of shared, the compiler guarantees that mutating a variable which 
_isn't_ shared won't alter any variables on another thread. By casting a 
variable to shared and passing it to another thread, you have violated that. 
It is undefined as to whether altering a variable which you passed to another 
thread like that will alter any other variable. It depends on whether any 
reference to the same data still exists on the original thread. So, the 
behavior - as with immutable - is undefined. It depends on what you've done in 
the code instead of on what the compiler guarantees.

In _both_ cases, you can change the variable. In both cases, the exact 
behavior is undefined. And in both cases, there's pretty much no way that 
there's any danger - on any compiler implementation - of it not working to 
pass the variable across like we've been discussing if no reference is kept on 
the original thread.

In any case, there's probably no point in arguing this any further. We 
obviously don't agree, and I don't think that either of us is really going to 
convince the other. Using immutable and shared both work in this case. You can 
use both and be perfectly fine as long as you're careful. As long as the 
original data is mutable and no reference to it is kept on the original 
thread, then both approaches work just fine.

I do think that it makes more sense to use shared in this case, since 
immutable _is_ shared anyway, and all we're really trying to do is get the 
data passed to another thread, not change its mutability. But we're obviously 
not going to agree on the details of what you are and aren't violating by 
casting back and forth with shared and immutable.

- Jonathan M Davis
Aug 19 2011
parent Timon Gehr <timon.gehr gmx.ch> writes:
On 08/20/2011 02:06 AM, Jonathan M Davis wrote:

 On Friday, August 19, 2011 15:27 Timon Gehr wrote:

 On 08/19/2011 10:36 PM, Jonathan M Davis wrote:

 On Friday, August 19, 2011 04:58 Timon Gehr wrote:

 On 08/19/2011 01:50 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 16:00 Timon Gehr wrote:

 On 08/19/2011 12:35 AM, Jonathan M Davis wrote:

 On Thursday, August 18, 2011 14:33 jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to
 change the data (otherwise std.typecons.assumeUnique would perform
 its job outside defined behavior btw)


 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language
 reference somewhere?

 I have no problem with using shared casts instead of immutable - I
 just want make sure it is really needed.


 The behavior of casting a way const or immutable on a value and then
 mutating it is undefined by the language, because you're breaking the
 language's guarantees and what happens depends entirely on whether
 the actual object was actually immutable. However, in the case of
 casting to immutable and then casting back, you _know_ that the
 object is mutable, so there's no problem. You're just circumventing
 the type system which throws away the guarantees that it gives you
 about immutability, which could screw up optimizations if you had
 actually did more than just pass the variable around. But that's
 just not happening here.

 As for casting to and from shared and mutating the object, I don't
 see how it is any more defined than casting to and from immutable
 and then mutating the object is. In both cases, you circumvented the
 type system, which breaks the compiler's guarantees and risks bugs
 if you actually do more than just pass the variable around before
 casting it back to being thread-local and mutable.

 - Jonathan M Davis


 As long as the data is not being shared between multiple threads after
 it's sharedness has been cast away, you are well in defined area,
 because you are NOT breaking anything.


 The _only_ reason that you're not breaking anything is because you are
 being careful and making sure that the data is not actually shared
 between threads. You're _still_ circumventing the type system and
 breaking the guarantee that a no>>>> Just to make sure we agree on that:
knowing why it is undefined does not


 imply there are cases where it is actually defined.n-shared variable is not
shared between

 threads. _You_ are the one guaranteeing that the variable is only on
 one thread, not the compiler. And when you cast away immutable, _you_
 are the one guaranteeing that immutable data is not being mutated, not
 the compiler. And in this case, you can make that guarantee in exactly
 the same way that you can guarantee that the variable which was cast
 to shared and back to be passed across threads isn't actually shared
 between threads once it has been passed.


 The crucial difference between immutable and shared is, that something
 that is immutable will always be immutable, but being shared or not
 may change dynamically.

 Casting to immutable is a one-way-street, while casting to shared is
 not.


 Casting to immutable does not make the data magically immutable.


 Yes it does. That is quite exactly the definition of it.


 It makes it
 so that the compiler treats it as immutable.


 The compiler is irrelevant, the fact that one compiler generates
 assembly that behaves as you'd like it to behave does not mean the code
 is valid.


 Casting from immutable makes it
 so that the compiler treats it as mutable. It does not alter whether
 the data is actually immutable.


 'Actually immutable' means that the variable is never changed. The
 language spec says that if an immutable variable is not 'actually
 immutable', your program is in error, even if immutability has been
 explicitly cast away.


 Casting away immutable and altering data is undefined,
 because it depends entirely on whether the data is actually immutable
 or not.


 Just to make sure we agree on that: knowing why it is undefined does not
 imply there are cases where it is actually defined.


 If it isn't, and you don't have any other references to the data (or
 none of the other references are immutable), then you don't break
 _anything_ by mutating the variable after having cast away immutable.


 If you believe the spec, you do break validity of your code even if the
 variable is not visible from another place after the cast. There is
 currently afaik no reason why such code would *have* to be broken, but
 that is what the spec says, because it categorically bans changing
 variables that were cast from immutable.


 With both shared and immutable, casting it away is circumnventing the
 type system. In both cases, _you_ must make sure that you code in a way
 which does not violate the guarantees that the compiler normally makes
 about those types. If you do violate those guarantees (e.g. by sharing
 non-shared data across threads or by mutating data which really was
 immutable), then you have a bug, and what happens is dependent on the
 compiler implementation and on your code.


 Agreed, and this means what happens is undefined and such code is
 invalid.


 But if you don't violate those guarantees, then your program is fine.
 It's the same for both shared and immutable. It's just that the bugs
 that you're likely to get when you violate those guarantees are likely
 to be quite different.

 - Jonathan M Davis


 Yes, I agree. And in this specific case you do violate the language
 guarantees about immutable variables when you change the variable after
 casting it back to mutable. Even if it was typed mutable sometime
 before, and even if it is the sole reference in the whole program.
 Therefore the behavior of the current implementation is undefined under
 the current spec, and changing the transfer protocol to use shared
 instead of immutable would fix this. (while generating near-identical
 assembly output with the current DMD)


 What I completely disagree with you on is that casting something to
 shared, passing it to another thread, casting from shared, and then
 altering a variable is any more defined than casting to and from
 immutable and altering a variable is. In _both_ cases, you are
 circumventing the compiler, and in _both_ cases, _you_ must guarantee
 that what you're doing doesn't actually violate what the compiler is
 trying to guarantee.

 - Jonathan M Davis


 As I said, in the case of immutable, changing the data is the problem:
 It does actually violate the rules of the spec. It is explicitly
 disallowed to change any data that has been cast from immutable. (still,
 with DMD it probably works.)

 shared has no such strong after-the-cast limitation (at least I haven't
 found a reason or a passage in the spec), the data should just not be
 visible by multiple threads afterwards. You may well change the variable
 after the cast.


 Casting away immutable on a variable and mutating it is _not_ disallowed by
 the spec. It is undefined as to what happens when you mutate the data after
 having cast away immutable.


I treat disallowed by the spec and undefined as synonyms.


 This is because it's completely dependent on what
 the actual state of the data was. If it's actually immutable (e.g. it's in
 ROM), then things are going to blow up on you. If it's not actually immutable,
 but there are other references to the data, then the mutation of the variable
 can cause bugs because the compiler makes assumptions based on the guarantees
 that immutable provides, and you broke those guarantees. So, as long as the
 variable was created as a mutable variable and there are no longer any
 immutable references to that data which could be relying on that data to not
 change, then there are _zero_ problems with casting away immutable and
 mutating a variable. It _is_ not disallowed to do that. It is merely
 _undefined_ as to what happens.


Timon Gehr wrote:

 Just to make sure we agree on that: knowing why it is undefined does 
 not imply there are cases where it is actually defined.




 If it were disallowed, then you couldn't do.


disallowed <=> undefined <=> code doing it is in error.
That it compiles does not mean it is allowed by the D spec.


 In the case of shared, the compiler guarantees that mutating a variable which
 _isn't_ shared won't alter any variables on another thread. By casting a
 variable to shared and passing it to another thread, you have violated that.


As long as it is the only reference on the first thread and is never 
read afterwards, no.

It is different for immutable and shared.

Timon Gehr wrote:

 http://www.digitalmars.com/d/2.0/const3.html
 See 'removing immutable with a cast'.


Here the spec is quite clear on that assigning to a variable that has 
been cast from const or immutable has no defined semantics.


 It is undefined as to whether altering a variable which you passed to another
 thread like that will alter any other variable. It depends on whether any
 reference to the same data still exists on the original thread. So, the
 behavior - as with immutable - is undefined. It depends on what you've done in
 the code instead of on what the compiler guarantees.


The semantics and validity of the code always depend on what you have 
done in the code.

Basically, as I understand it, you are saying that any code that 
contains a cast that is potentially unsafe is undefined. That is like 
saying that any function not annotated with  safe does corrupt memory. 
It also implies that you cannot use std.typecons.assumeUnique correcly.

btw: "undefined" and "dependent on what you have done in your code" 
cannot be used interchangeably.


 In _both_ cases, you can change the variable. In both cases, the exact
 behavior is undefined.


In case of immutable, the spec specifies it is undefined in all cases. 
For shared whether it is defined and the exact behavior is dependent on 
what the rest of the code does.


 And in both cases, there's pretty much no way that
 there's any danger - on any compiler implementation - of it not working to
 pass the variable across like we've been discussing if no reference is kept on
 the original thread.


A compiler implementation could do this:

auto foo = cast(immutable) bar;
// insert OS-call to write-protect the memory page

and still be up to the spec. I am not arguing about anything that is 
very relevant in the real world. I am just arguing about what the spec says:

As I understand it, the spec says:
1. immutable data is _never_ going to change once it has been initialized.
2. unshared data is not being shared between multiple threads.

3. casting data from immutable is fine and defined, as long as it is not 
changed. If it is changed, the behavior is undefined. (NOT 'dependent on 
the rest of the program'. Because the program is right there, that would 
actually mean the behavior is defined).

4. There is actually no page (which I have found) describing any such 
overly strong implications of casting to and from shared types. So you 
are fine if you can guarantee that unshared variables will never be 
visible by multiple threads. (where not reading a variable until the 
memory is released counts as invisible)


 In any case, there's probably no point in arguing this any further. We
 obviously don't agree, and I don't think that either of us is really going to
 convince the other.


Either that, or we agree, but are interpreting the meaning of some terms 
quite differently.


 Using immutable and shared both work in this case. You can
 use both and be perfectly fine as long as you're careful. As long as the
 original data is mutable and no reference to it is kept on the original
 thread, then both approaches work just fine.

 I do think that it makes more sense to use shared in this case, since
 immutable _is_ shared anyway, and all we're really trying to do is get the
 data passed to another thread, not change its mutability. But we're obviously
 not going to agree on the details of what you are and aren't violating by
 casting back and forth with shared and immutable.


Ok, I'm fine with stopping to argue.
Apparently there is a funny bug in Phobos that prevents using shared 
anyways.
Aug 20 2011
prev sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 08/18/2011 11:33 PM, jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to change
 the data (otherwise std.typecons.assumeUnique would perform its job
 outside defined behavior btw)


 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?

 I have no problem with using shared casts instead of immutable - I just
 want make sure it is really needed.


Yes.

http://www.digitalmars.com/d/2.0/const3.html
See 'removing immutable with a cast'.

It basically says that your program is in error if it changes data whose 
immutability has been cast away.

If it's 'really needed' depends on what you consider 'really needed'. It 
will work as intended with immutable (and the current DMD compiler 
implementation and probably most implementations of the language that 
there will be), but the code will still be incorrect.
Aug 18 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 19-08-2011 00:55, Timon Gehr skrev:

 On 08/18/2011 11:33 PM, jdrewsen wrote:

 Den 17-08-2011 18:21, Timon Gehr skrev:

 On 08/17/2011 05:58 PM, Jonathan M Davis wrote:

 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.


 No. As soon as the data is typed as immutable anywhere it cannot be
 changed anymore. You only break guarantees if you actually try to change
 the data (otherwise std.typecons.assumeUnique would perform its job
 outside defined behavior btw)


 I'm thinking down the same lines as Jonathan. Is the behavior for
 immutable casts that you describe specified in the language reference
 somewhere?

 I have no problem with using shared casts instead of immutable - I just
 want make sure it is really needed.


 Yes.

 http://www.digitalmars.com/d/2.0/const3.html
 See 'removing immutable with a cast'.

 It basically says that your program is in error if it changes data whose
 immutability has been cast away.

 If it's 'really needed' depends on what you consider 'really needed'. It
 will work as intended with immutable (and the current DMD compiler
 implementation and probably most implementations of the language that
 there will be), but the code will still be incorrect.


I don't think the documentation is unambiguous in this matter. Anyway I 
tried to change it to shared but it doesn't seem to work:

Works:
     workerTid.send(cast(immutable(char)[])arr);

Doesn't work:
     workerTid.send(cast(shared(char)[])arr);

.../std/variant.d(528): Error: function core.stdc.string.memcpy (void* 
s1, in const(void*) s2, uint n) is not callable using argument types 
(ubyte[24u]*,shared(char[])*,uint)
.../std/variant.d(528): Error: cannot implicitly convert expression (& 
rhs) of type shared(char[])* to const(void*)
make: *** [all] Error 1

I don't know if this is a bug in std.variant?

/Jonas
Aug 19 2011
prev sibling parent reply jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 15:13, dsimcha skrev:

 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?


It uses the spawn method from std.concurrency which uses D threads i think.


 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?


Not really. It is just an example and writeln will output a byte array 
just fine.


 For onReceive, what's the purpose of the return value?


To specify the number of bytes that have been handled. If this is less 
that the entire buffer the rest of the request will fail. Additionally 
both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE 
pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return 
CURL_READFUNC_ABORT to abort as well. I see that the is missing from the 
docs... will add.


 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 think that would be convenient but don't believe it is the correct 
thing to do. In my opinion D modules that are not pure D or wrapping a 
system library (which libcurl is definitely not) should be the only ones 
allowed in the std package. Other wrappers in phobos should be in the 
etc package and the lib binaries should not be included.

Just how I feel about it though. Anyone who disagrees?

/Jonas
Aug 16 2011
next sibling parent reply dsimcha <dsimcha gmail.com> writes:
== Quote from jdrewsen (jdrewsen nospam.com)'s article

 Den 16-08-2011 15:13, dsimcha skrev:

 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?


 It uses the spawn method from std.concurrency which uses D threads i think.

 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?


 Not really. It is just an example and writeln will output a byte array
 just fine.

 For onReceive, what's the purpose of the return value?


 To specify the number of bytes that have been handled. If this is less
 that the entire buffer the rest of the request will fail. Additionally
 both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE
 pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return
 CURL_READFUNC_ABORT to abort as well. I see that the is missing from the
 docs... will add.

 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 think that would be convenient but don't believe it is the correct
 thing to do. In my opinion D modules that are not pure D or wrapping a
 system library (which libcurl is definitely not) should be the only ones
 allowed in the std package. Other wrappers in phobos should be in the
 etc package and the lib binaries should not be included.
 Just how I feel about it though. Anyone who disagrees?
 /Jonas


Ok, this makes more sense if we define the etc package to be wrappers and
binding
for non-D libraries.  I thought this thing was going into std, in which case it
should feel, well, standard.  Is that officially how etc is defined?
Aug 16 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 20:49, dsimcha skrev:

 == Quote from jdrewsen (jdrewsen nospam.com)'s article

 Den 16-08-2011 15:13, dsimcha skrev:

 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?


 It uses the spawn method from std.concurrency which uses D threads i think.

 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?


 Not really. It is just an example and writeln will output a byte array
 just fine.

 For onReceive, what's the purpose of the return value?


 To specify the number of bytes that have been handled. If this is less
 that the entire buffer the rest of the request will fail. Additionally
 both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE
 pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return
 CURL_READFUNC_ABORT to abort as well. I see that the is missing from the
 docs... will add.

 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 think that would be convenient but don't believe it is the correct
 thing to do. In my opinion D modules that are not pure D or wrapping a
 system library (which libcurl is definitely not) should be the only ones
 allowed in the std package. Other wrappers in phobos should be in the
 etc package and the lib binaries should not be included.
 Just how I feel about it though. Anyone who disagrees?
 /Jonas


 Ok, this makes more sense if we define the etc package to be wrappers and
binding
 for non-D libraries.  I thought this thing was going into std, in which case it
 should feel, well, standard.  Is that officially how etc is defined?


I don't think there is an official definition yet. But I would like to 
suggest the setup described to be official one.
Aug 16 2011
prev sibling next sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, August 16, 2011 20:23:19 jdrewsen wrote:

 Den 16-08-2011 15:13, dsimcha skrev:

 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?


 
 It uses the spawn method from std.concurrency which uses D threads i think.
 

 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?


 
 Not really. It is just an example and writeln will output a byte array
 just fine.
 

 For onReceive, what's the purpose of the return value?


 
 To specify the number of bytes that have been handled. If this is less
 that the entire buffer the rest of the request will fail. Additionally
 both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE
 pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return
 CURL_READFUNC_ABORT to abort as well. I see that the is missing from the
 docs... will add.
 

 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 think that would be convenient but don't believe it is the correct
 thing to do. In my opinion D modules that are not pure D or wrapping a
 system library (which libcurl is definitely not) should be the only ones
 allowed in the std package. Other wrappers in phobos should be in the
 etc package and the lib binaries should not be included.
 
 Just how I feel about it though. Anyone who disagrees?


In general, external libraries should not be included in Phobos. They should 
be using whatever is on the user's system. As such, you're correct in that etc 
is probably the place to put the curl wrapper, not std.

- Jonathan M Davis
Aug 16 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 21:59, Jonathan M Davis skrev:

 On Tuesday, August 16, 2011 20:23:19 jdrewsen wrote:

 Den 16-08-2011 15:13, dsimcha skrev:

 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?


 It uses the spawn method from std.concurrency which uses D threads i think.


 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?


 Not really. It is just an example and writeln will output a byte array
 just fine.


 For onReceive, what's the purpose of the return value?


 To specify the number of bytes that have been handled. If this is less
 that the entire buffer the rest of the request will fail. Additionally
 both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE
 pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return
 CURL_READFUNC_ABORT to abort as well. I see that the is missing from the
 docs... will add.


 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 think that would be convenient but don't believe it is the correct
 thing to do. In my opinion D modules that are not pure D or wrapping a
 system library (which libcurl is definitely not) should be the only ones
 allowed in the std package. Other wrappers in phobos should be in the
 etc package and the lib binaries should not be included.

 Just how I feel about it though. Anyone who disagrees?


 In general, external libraries should not be included in Phobos. They should
 be using whatever is on the user's system. As such, you're correct in that etc
 is probably the place to put the curl wrapper, not std.

 - Jonathan M Davis


It could ofcourse just be make a root module 'curl' instead of 
'etc.curl'. I would like it to be etc.curl in phobos since I think D 
needs a set of "Phobos approved" extensions. Extensions that are 
guaranteed to have certain amount of quality because they have been 
through the review process.

Maybe at some point when a package manager of some kind is available the 
modules in the etc package can be torn out of the official phobos bundle 
and downloaded separately from d-p-l.org.

/Jonas
Aug 16 2011
prev sibling parent reply bearophile <bearophileHUGS lycos.com> writes:
jdrewsen:


 Other wrappers in phobos should be in the 
 etc package and the lib binaries should not be included.
 
 Just how I feel about it though. Anyone who disagrees?


All necessary binaries of the libraries used by all Phobos modules need to be
in the zip of the Windows distribution (and probably all other binary
distributions of DMD too). Otherwise they are not worth adding to Phobos.

Bye,
bearophile
Aug 16 2011
parent reply jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 22:05, bearophile skrev:

 jdrewsen:


 Other wrappers in phobos should be in the
 etc package and the lib binaries should not be included.

 Just how I feel about it though. Anyone who disagrees?


 All necessary binaries of the libraries used by all Phobos modules need to be
in the zip of the Windows distribution (and probably all other binary
distributions of DMD too). Otherwise they are not worth adding to Phobos.

 Bye,
 bearophile


etc.c.curl is already in phobos and no binary library for curl is in.

I disagree on this one though. But please see my reply to jonathan as to 
how I see final solution.

/Jonas
Aug 16 2011
parent reply bearophile <bearophileHUGS lycos.com> writes:
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

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.

Bye,
bearophile
Aug 16 2011
next sibling parent Jesse Phillips <jessekphillips+d gmail.com> writes:
On Tue, 16 Aug 2011 16:39:36 -0400, 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
 
 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.
 
 Bye,
 bearophile


My original SQLite3 pull request included the SQLite library and had it 
compiled into Phobos just as zlib. This was turned down from the 
perspective that bug fixes to SQLite will not require an update to Phobos.

While not everyone voice an opinion no one but myself was arguing to 
include the library.
Aug 16 2011
prev sibling parent reply Jonas Drewsen <jdrewsen nospam.com> writes:
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
Aug 17 2011
parent reply bearophile <bearophileHUGS lycos.com> writes:
Jonas Drewsen:


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?<


What I think is acceptable is to put now the binary of the Windows library
inside the zip of the DMD distribution. Once that decent package management
tool will be present and working on Windows, you will be free to remove the
binary lib, if you want (but I suggest to not do it, because D std library is
suppsed to have few batteries included, and today accessing the web is a basic
need. Maybe even the package management tool needs a curl to download the
files, so we are back to the start).

----------------

dsimcha:
	

Also, anyone have any luck getting Curl libs to work with DMD on Windows?  The
binaries available at http://curl.haxx.se/download.html look like they're COFF.
 (<rant> Why do we still use freakin' OMF? </rant>)  Also, the source doesn't
look like it's pleasant to compile.<


See my answer above :-) If I want to use DMD to write a 20 lines long
script-line program I will not compile a C lib (and probably a 200 lines long
program too).

Bye,
bearophile
Aug 18 2011
next sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, August 18, 2011 04:37:20 bearophile wrote:

 Jonas Drewsen:

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?<


 What I think is acceptable is to put now the binary of the Windows library
 inside the zip of the DMD distribution. Once that decent package management
 tool will be present and working on Windows, you will be free to remove the
 binary lib, if you want (but I suggest to not do it, because D std library
 is suppsed to have few batteries included, and today accessing the web is a
 basic need. Maybe even the package management tool needs a curl to download
 the files, so we are back to the start).
 
 ----------------
 
 dsimcha:

Also, anyone have any luck getting Curl libs to work with DMD on Windows? 
The binaries available at http://curl.haxx.se/download.html look like
they're COFF.  (<rant> Why do we still use freakin' OMF? </rant>)  Also,
the source doesn't look like it's pleasant to compile.<


 See my answer above :-) If I want to use DMD to write a 20 lines long
 script-line program I will not compile a C lib (and probably a 200 lines
 long program too).


It would be a good idea to provide links to to such a binary (probably in the 
documentation), but I definitely don't think that any such thing should be 
provided with dmd itself. It increases the size of the zip file unnecessarily 
for the majority of users, and anyone using anything other than Windows 
definitely won't need it.

So yes, we need to make it easy for people to find the appropriate binaries for 
Windows, but no, we shouldn't distribute them with dmd.

- Jonathan M Davis
Aug 18 2011
parent reply bearophile <bearophileHUGS lycos.com> writes:
Jonathan M Davis:


It increases the size of the zip file unnecessarily for the majority of users,
and anyone using anything other than Windows definitely won't need it.<


This is a silly argument. Have you seen the size of the binary distributions of
Python, Java, DotNet? The Haskell installer is 93.6 MB. Java jdk-7-ea-bin is
about 100 MB. The DMD zip is very far from there. If this is not acceptable for
you still, I see solutions like distribuiting zip with library binaries too and
a bare-bones zip for you. Is this good enough for you?



So yes, we need to make it easy for people to find the appropriate binaries for
Windows, but no, we shouldn't distribute them with dmd.<


Downloading files from the net is a common task for even short programs. To
lower the entry barrier for D newbies you don't want them to download (or
compile) libs.

Bye,
bearophile
Aug 18 2011
next sibling parent reply dsimcha <dsimcha yahoo.com> writes:
On 8/18/2011 8:16 AM, bearophile wrote:

 Jonathan M Davis:


 It increases the size of the zip file unnecessarily for the majority of users,
and anyone using anything other than Windows definitely won't need it.<


 This is a silly argument. Have you seen the size of the binary distributions
of Python, Java, DotNet? The Haskell installer is 93.6 MB. Java jdk-7-ea-bin is
about 100 MB. The DMD zip is very far from there. If this is not acceptable for
you still, I see solutions like distribuiting zip with library binaries too and
a bare-bones zip for you. Is this good enough for you?


...That and we could get the zip file size back down by using 
OS-specific zips.  You know what Linux users really don't need?  Windows 
DMD and Phobos binaries.
Aug 18 2011
parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 8/18/11, dsimcha <dsimcha yahoo.com> wrote:

 ...That and we could get the zip file size back down by using
 OS-specific zips.  You know what Linux users really don't need?  Windows
 DMD and Phobos binaries.


And vice versa.
Aug 18 2011
prev sibling parent "Marco Leise" <Marco.Leise gmx.de> writes:
Am 18.08.2011, 14:16 Uhr, schrieb bearophile <bearophileHUGS lycos.com>:


 Jonathan M Davis:


 It increases the size of the zip file unnecessarily for the majority of  
 users, and anyone using anything other than Windows definitely won't  
 need it.<


 This is a silly argument. Have you seen the size of the binary  
 distributions of Python, Java, DotNet? The Haskell installer is 93.6 MB.  
 Java jdk-7-ea-bin is about 100 MB. The DMD zip is very far from there.  
 If this is not acceptable for you still, I see solutions like  
 distribuiting zip with library binaries too and a bare-bones zip for  
 you. Is this good enough for you?


Don't be sarcastic. A smaller zip file reduces costs (and improves the  
download times) and I'm only interested in the sources anyway. The way you  
put it I don't know if you are actually trying to make a joke? Since  
Windows does not have a dependency management through some package manager  
you end up with lots of duplicate DLLs and have to include them in your  
downloads, too. On other platforms this is for good reasons handled  
differently and you benefit from bug fixes in libraries, too when you  
declare a dependency on Python 2, >= 2.6 for example.

The examples you name are no good counter-argument to unnecessary file  
bloat due to bundled libraries. I cannot install the (official) .NET  
platform on my Linux, but neither Java nor Haskell have that file size  
 from third party libraries they bundle. Like .NET they like to call  
themselves 'platform' and include their own APIs that work on every  
supported OS. Over time the runtime for those simply grew over time with  
the oldest listed download in Oracle's archive for Java 1.1.6 being 5 MB  
in size. If you take a look at the Python 3 source download, it is just  
around 10 MB, because Python instead uses wrappers for existing libraries,  
with most of the rarely used stuff not bundled.

So Java and Haskell are still huge downloads. Yes, but while these  
download I don't think that I am downloading mostly duplicates of  
libraries that are already installed as dependencies or system base  
packages. These 'platforms' come with HTTP functionality, that is  
different and unique from curl and most probably works directly on the  
system's sockets implementation. The benefit is probably that their API is  
more consistent and different modules can work together. For example there  
may be HttpInputStreams that are interchangeable with other InputStreams  
or wrappable in encryption filters etc. So for the effort that was put  
into the 100 MB download and the consistent API I'm ok with the download  
size.

To come to an end I propose that the
- 'all platforms' download becomes a source only package
- the Windows download includes binaries for libraries used in Phobos or a  
statically linked mega-Phobos that outgrows Mars somewhat


- Marco
Aug 18 2011
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-08-18 10:37, bearophile wrote:

 Jonas Drewsen:


 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?<


 What I think is acceptable is to put now the binary of the Windows library
inside the zip of the DMD distribution. Once that decent package management
tool will be present and working on Windows, you will be free to remove the
binary lib, if you want (but I suggest to not do it, because D std library is
suppsed to have few batteries included, and today accessing the web is a basic
need. Maybe even the package management tool needs a curl to download the
files, so we are back to the start).


Curl could be statically linked and the package manager would have no 
problem.


 ----------------

 dsimcha:
 	

 Also, anyone have any luck getting Curl libs to work with DMD on Windows?  The
binaries available at http://curl.haxx.se/download.html look like they're COFF.
 (<rant>  Why do we still use freakin' OMF?</rant>)  Also, the source doesn't
look like it's pleasant to compile.<


 See my answer above :-) If I want to use DMD to write a 20 lines long
script-line program I will not compile a C lib (and probably a 200 lines long
program too).

 Bye,
 bearophile



-- 
/Jacob Carlborg
Aug 18 2011
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 8/18/11 6:48 AM, Jacob Carlborg wrote:

 On 2011-08-18 10:37, bearophile wrote:

 Jonas Drewsen:


 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?<


 What I think is acceptable is to put now the binary of the Windows
 library inside the zip of the DMD distribution. Once that decent
 package management tool will be present and working on Windows, you
 will be free to remove the binary lib, if you want (but I suggest to
 not do it, because D std library is suppsed to have few batteries
 included, and today accessing the web is a basic need. Maybe even the
 package management tool needs a curl to download the files, so we are
 back to the start).


 Curl could be statically linked and the package manager would have no
 problem.


I think we need to include curl with the Windows distribution. It's 
reasonable to assume connectivity is a broadly needed feature. We must 
solve the zip bloating issue for Unix people by breaking the zip per 
platform. Next to nobody ever wants to have Windows, Linux, FreeBSD, 
OSX, etc. files in the same distribution.


Andrei
Aug 18 2011
parent kennytm <kennytm gmail.com> writes:
Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> wrote:


 I think we need to include curl with the Windows distribution. It's
 reasonable to assume connectivity is a broadly needed feature. We must
 solve the zip bloating issue for Unix people by breaking the zip per
 platform. Next to nobody ever wants to have Windows, Linux, FreeBSD, OSX,
 etc. files in the same distribution.
 
 
 Andrei


+1.

I've seen the per-platform zip suggestions many times, but still till now
no actions are taken. :( </whine>
Aug 18 2011
prev sibling next sibling parent reply Alix Pexton <alix.DOT.pexton gmail.DOT.com> writes:
On 16/08/2011 12:48, 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



Here are my initial observations and impressions...

In the opening description...
"The request is perform when first accessing..." -> "The request is 
performed when first accessing..."

On first reading, I found it very hard to follow the first collected 
example. (Still not 100% clear after reading the whole of the 
documentation either >< )

Some (all?) of the  property members of Protocol are documented as 
functions. (Perhaps an artefact of the ddoc mixin issue mentioned before 
this section?)

Description of Protocol.onSend...
"...specifies the max number of bytes that can be send." -> 
"...specifies the maximum number of bytes that can be sent."
"...returns the number of elements in the buffer that has been filled 
and is ready to send." -> "...returns the number of elements in the 
buffer that have been filled and are ready to send."

Description of Protocol.onReceive...
"...is not guaranteed to be valid aften the callback returns." -> "...is 
not guaranteed to be valid after the callback returns."

Protocol.onProgress represents transfer statistics using doubles?

CurlTimeCond, members are all lower case, took me a few goes to work out 
what they might mean, still not all that sure. Should I assume that if 
the meanings are not obvious that I'm not ready to be tinkering with 
curl at this level, or should each value be clarified?

Http.addHeader / Http.setHeader both have the same description, I assume 
the latter overwrites an existing value if it exists? what is the 
behaviour of addHeader when the key already exists?

Http.setTimeCondition parameter names in ddoc don't match those in the 
function prototype.

Http._whatever_Async (and also Async functions for Ftp.)
"Asynchronous HTTP _WHATEVER_ to the specified URL." -> "Performs a HTTP 
_WHATEVER_ on the specified URL, asynchronously."
(Wording looks very similar across this group of functions, not sure if 
my suggested alteration here correctly captures the subtleties for all 
cases. Also, later examples are just marked as "Asynchronous version 
of..." and lose the note about callbacks, which I assume still apply.)

All callbacks are write only properties which prevents the use of event 
hooking (which, in my experience, is a common occurrence in event based 
models). Hooking might not apply in this case however (I've not 
particularly thought through any use cases) and safe event hooking might 
be beyond the scope of this module.

Http.traceAsync
Calls AsyncResult with Method.get? I would have expected Method.trace, 
but I only spotted this by chance, could very well be my ignorance 
showing ^^

Http.onReceiveStatusLine
"...several callbacks can be done for on call to perform() because if 
redirections." -> "...several callbacks can be done for each call to 
perform() [because of | due to] redirections."

Http.authenticationMethod
std.etc.c.curl.AuthMethod? I think this is an issue with the XREF macro 
not expecting the link to break out of the std package.

Ftp.Result.encoding / Ftp.AsyncResult.encoding
I'm not sure what these functions are supposed to return, the 
descriptions seem inadequate.

---

Not fully read all the source yet, but as I'm not all that familiar with 
curl, I'm not sure I'll spot much ^^

Not a criticism, but at first glance, it doesn't feel very D-ey, but I'm 
still not 100% sure what idiomatic D is yet.

A...
Aug 16 2011
next sibling parent kennytm <kennytm gmail.com> writes:
Alix Pexton <alix.DOT.pexton gmail.DOT.com> wrote:


 
 Some (all?) of the  property members of Protocol are documented as
 functions. (Perhaps an artefact of the ddoc mixin issue mentioned before this
section?)


Bug 3445. 
 

 Protocol.onProgress represents transfer statistics using doubles?


Unfortunately curl's API uses double.
Aug 16 2011
prev sibling next sibling parent reply "Nick Sabalausky" <a a.a> writes:
"Alix Pexton" <alix.DOT.pexton gmail.DOT.com> wrote in message 
news:j2dsq0$2aht$1 digitalmars.com...

 "...returns the number of elements in the buffer that has been filled and 
 is ready to send." -> "...returns the number of elements in the buffer 
 that have been filled and are ready to send."


"in the buffer that has been filled" is correct. The "has" refers to "the 
buffer" not the elements that have been put into it. And there's only one 
buffer in question, so "has".

But yea, "is ready to send" should be changed to "are ready to send".



 "Asynchronous HTTP _WHATEVER_ to the specified URL." -> "Performs a HTTP 
 _WHATEVER_ on the specified URL, asynchronously."


"Performs a HTTP" -> "Performs an HTTP". The letter "H" may technically be a 
consonant, but in this case it's read as the *name* of the letter, not the 
letter's normal sound. And the name of H starts with a vowel sound (long A). 
(Besides, "a HTTP" just sounds awkward.)

Also, little bikeshedding, but I think this would be better still:

"Performs an asynchronous HTTP _WHATEVER_ on the specified URL."
Aug 16 2011
next sibling parent jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 18:38, Nick Sabalausky skrev:

 "Alix Pexton"<alix.DOT.pexton gmail.DOT.com>  wrote in message
 news:j2dsq0$2aht$1 digitalmars.com...

 "...returns the number of elements in the buffer that has been filled and
 is ready to send." ->  "...returns the number of elements in the buffer
 that have been filled and are ready to send."


 "in the buffer that has been filled" is correct. The "has" refers to "the
 buffer" not the elements that have been put into it. And there's only one
 buffer in question, so "has".

 But yea, "is ready to send" should be changed to "are ready to send".


ok


 "Asynchronous HTTP _WHATEVER_ to the specified URL." ->  "Performs a HTTP
 _WHATEVER_ on the specified URL, asynchronously."


 "Performs a HTTP" ->  "Performs an HTTP". The letter "H" may technically be a
 consonant, but in this case it's read as the *name* of the letter, not the
 letter's normal sound. And the name of H starts with a vowel sound (long A).
 (Besides, "a HTTP" just sounds awkward.)


ok


 Also, little bikeshedding, but I think this would be better still:

 "Performs an asynchronous HTTP _WHATEVER_ on the specified URL."
Aug 16 2011
prev sibling parent Alix Pexton <alix.DOT.pexton gmail.DOT.com> writes:
On 16/08/2011 17:38, Nick Sabalausky wrote:

 "Alix Pexton"<alix.DOT.pexton gmail.DOT.com>  wrote in message
 news:j2dsq0$2aht$1 digitalmars.com...

 "...returns the number of elements in the buffer that has been filled and
 is ready to send." ->  "...returns the number of elements in the buffer
 that have been filled and are ready to send."


 "in the buffer that has been filled" is correct. The "has" refers to "the
 buffer" not the elements that have been put into it. And there's only one
 buffer in question, so "has".

 But yea, "is ready to send" should be changed to "are ready to send".


I suspected my edit was off in this case (from a technical point of 
view), but it is still poor use of English (imho). If the buffer has 
been "filled" then the return will equal its capacity, otherwise filled 
seems to be the wrong word. Perhaps changed "filled" to "written"?


 "Asynchronous HTTP _WHATEVER_ to the specified URL." ->  "Performs a HTTP
 _WHATEVER_ on the specified URL, asynchronously."


 "Performs a HTTP" ->  "Performs an HTTP". The letter "H" may technically be a
 consonant, but in this case it's read as the *name* of the letter, not the
 letter's normal sound. And the name of H starts with a vowel sound (long A).
 (Besides, "a HTTP" just sounds awkward.)

 Also, little bikeshedding, but I think this would be better still:

 "Performs an asynchronous HTTP _WHATEVER_ on the specified URL."


I played with this word order too, but I must admit the a/an thing snook 
through between many of my rewrites, good catch ^^

A...
Aug 16 2011
prev sibling parent reply jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 15:54, Alix Pexton skrev:

 On 16/08/2011 12:48, 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



 Here are my initial observations and impressions...

 In the opening description...
 "The request is perform when first accessing..." -> "The request is
 performed when first accessing..."


ok


 On first reading, I found it very hard to follow the first collected
 example. (Still not 100% clear after reading the whole of the
 documentation either >< )


The first collected example? Can you tell me the comment above the 
example you do not understand?


 Some (all?) of the  property members of Protocol are documented as
 functions. (Perhaps an artefact of the ddoc mixin issue mentioned before
 this section?)


This is how it is for all modules AFAIK.


 Description of Protocol.onSend...
 "...specifies the max number of bytes that can be send." ->
 "...specifies the maximum number of bytes that can be sent."
 "...returns the number of elements in the buffer that has been filled
 and is ready to send." -> "...returns the number of elements in the
 buffer that have been filled and are ready to send."


ok


 Description of Protocol.onReceive...
 "...is not guaranteed to be valid aften the callback returns." -> "...is
 not guaranteed to be valid after the callback returns."


ok


 Protocol.onProgress represents transfer statistics using doubles?


That is what we get from libcurl


 CurlTimeCond, members are all lower case, took me a few goes to work out
 what they might mean, still not all that sure. Should I assume that if
 the meanings are not obvious that I'm not ready to be tinkering with
 curl at this level, or should each value be clarified?


I will include a link to the HTTP RFC that explains it.


 Http.addHeader / Http.setHeader both have the same description, I assume
 the latter overwrites an existing value if it exists? what is the
 behaviour of addHeader when the key already exists?


Added explanation and a link to HTTP RFC explaining sematics.


 Http.setTimeCondition parameter names in ddoc don't match those in the
 function prototype.


ok


 Http._whatever_Async (and also Async functions for Ftp.)
 "Asynchronous HTTP _WHATEVER_ to the specified URL." -> "Performs a HTTP
 _WHATEVER_ on the specified URL, asynchronously."
 (Wording looks very similar across this group of functions, not sure if
 my suggested alteration here correctly captures the subtleties for all
 cases.


ok


 Also, later examples are just marked as "Asynchronous version
 of..." and lose the note about callbacks, which I assume still apply.)


The latter examples are trivial HTTP methods not sending or receiving 
anything but the status codes are used. Therefore they do not need an 
elaborate example as for the GET/POST/PUT methods.


 All callbacks are write only properties which prevents the use of event
 hooking (which, in my experience, is a common occurrence in event based
 models). Hooking might not apply in this case however (I've not
 particularly thought through any use cases) and safe event hooking might
 be beyond the scope of this module.


It is possible to provide read functionallity of the properties. The 
only issue with this is that the delegate read would not be the same as 
the one just written because it gets wrapped it the propery set call. I 
don't know if this is a problem though?


 Http.traceAsync
 Calls AsyncResult with Method.get? I would have expected Method.trace,
 but I only spotted this by chance, could very well be my ignorance
 showing ^^


That's a bug. Will fix.


 Http.onReceiveStatusLine
 "...several callbacks can be done for on call to perform() because if
 redirections." -> "...several callbacks can be done for each call to
 perform() [because of | due to] redirections."


ok


 Http.authenticationMethod
 std.etc.c.curl.AuthMethod? I think this is an issue with the XREF macro
 not expecting the link to break out of the std package.


That ddoc standard config needs a patch to support etc module xrefs.


 Ftp.Result.encoding / Ftp.AsyncResult.encoding
 I'm not sure what these functions are supposed to return, the
 descriptions seem inadequate.


I will try to improve the docs. They return the EncodingSchemes that is 
used when reading the data

http://www.d-programming-language.org/phobos/std_encoding.html

The scheme is changed by using the encodingName property.


 ---

 Not fully read all the source yet, but as I'm not all that familiar with
 curl, I'm not sure I'll spot much ^^

 Not a criticism, but at first glance, it doesn't feel very D-ey, but I'm
 still not 100% sure what idiomatic D is yet.


Maybe you're right. Whether it is a product of being a wrapper around an 
existing library and thereby forcing some design decisions or by my 
coding style I don't know.

If anyone can give some input as how to make it more D'ish please do.

Thank you the valuable feedback
/Jonas


 A...
Aug 16 2011
parent Alix Pexton <alix.DOT.pexton gmail.DOT.com> writes:
On 16/08/2011 20:44, jdrewsen wrote:

 Den 16-08-2011 15:54, Alix Pexton skrev:


 Protocol.onProgress represents transfer statistics using doubles?


 That is what we get from libcurl


I'll let you off then, but just this once ^^


 CurlTimeCond, members are all lower case, took me a few goes to work out
 what they might mean, still not all that sure. Should I assume that if
 the meanings are not obvious that I'm not ready to be tinkering with
 curl at this level, or should each value be clarified?


 I will include a link to the HTTP RFC that explains it.


Seems reasonable.


 Http.addHeader / Http.setHeader both have the same description, I assume
 the latter overwrites an existing value if it exists? what is the
 behaviour of addHeader when the key already exists?


 Added explanation and a link to HTTP RFC explaining sematics.


I read the source now, they do do as I expected, which made me wonder 
why there is no removeHeader, but I can't think of a reason to use 
removeHeader, so I won't suggest its addition ^^


 Also, later examples are just marked as "Asynchronous version
 of..." and lose the note about callbacks, which I assume still apply.)


 The latter examples are trivial HTTP methods not sending or receiving
 anything but the status codes are used. Therefore they do not need an
 elaborate example as for the GET/POST/PUT methods.


Well, my honest thought as I read through it was that you were getting 
fed up of writing near identical documentation for each method and had 
started to cut corners. Can ddoc be used to add a section in front of 
these functions that outlines their common features? Making it a bit DRYer?


 All callbacks are write only properties which prevents the use of event
 hooking (which, in my experience, is a common occurrence in event based
 models). Hooking might not apply in this case however (I've not
 particularly thought through any use cases) and safe event hooking might
 be beyond the scope of this module.


 It is possible to provide read functionallity of the properties. The
 only issue with this is that the delegate read would not be the same as
 the one just written because it gets wrapped it the propery set call. I
 don't know if this is a problem though?


Hmn, it could well be! I've not thought of a good use case yet though, 
so I'd recommend leaving it as it is for now, and if D gets an Events 
module in the future, the hooking issue can be revisited.


 Ftp.Result.encoding / Ftp.AsyncResult.encoding
 I'm not sure what these functions are supposed to return, the
 descriptions seem inadequate.


 I will try to improve the docs. They return the EncodingSchemes that is
 used when reading the data

 http://www.d-programming-language.org/phobos/std_encoding.html

 The scheme is changed by using the encodingName property.


I looked at the code, and for a moment I though I got it, but I was wrong ><
from the source...

 ref Result encoding(const(char)[] schemeName) {
             rp._encodingSchemeName = schemeName;
             return this;
         }


I'm now guessing this is just for chaining?


 Not fully read all the source yet, but as I'm not all that familiar with
 curl, I'm not sure I'll spot much ^^

 Not a criticism, but at first glance, it doesn't feel very D-ey, but I'm
 still not 100% sure what idiomatic D is yet.


 Maybe you're right. Whether it is a product of being a wrapper around an
 existing library and thereby forcing some design decisions or by my
 coding style I don't know.

 If anyone can give some input as how to make it more D'ish please do.


The source, for the most part, does feel D'ish, so you must be doing 
something right, but there is no point in adding extra templates and 
custom ranges if there is no need. I'll have a stab at writing some code 
with it tomorrow, to see how the bits fit together in the wild.


 Thank you the valuable feedback


No problem, I'm glad you found it so ^^

A...
Aug 16 2011
prev sibling next sibling parent reply kennytm <kennytm gmail.com> writes:
Jonas Drewsen <jdrewsen nospam.com> 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


1) Why 'Http' and 'Ftp' but 'SMTP' is in all caps?
2) Some links in the doc says 'std.curl....'
Aug 16 2011
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-08-16 15:54, kennytm wrote:

 Jonas Drewsen<jdrewsen nospam.com>  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


 1) Why 'Http' and 'Ftp' but 'SMTP' is in all caps?
 2) Some links in the doc says 'std.curl....'


BTW, do we have style guides for abbreviations like this?

-- 
/Jacob Carlborg
Aug 16 2011
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, August 16, 2011 18:47:44 Jacob Carlborg wrote:

 On 2011-08-16 15:54, kennytm wrote:

 Jonas Drewsen<jdrewsen nospam.com>  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


 
 1) Why 'Http' and 'Ftp' but 'SMTP' is in all caps?
 2) Some links in the doc says 'std.curl....'


 
 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
Aug 16 2011
parent reply Jacob Carlborg <doob me.com> writes:
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
Aug 17 2011
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
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
Aug 17 2011
parent Jacob Carlborg <doob me.com> writes:
On 2011-08-17 09:13, Jonathan M Davis wrote:

 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


Ok, seems we agree.

-- 
/Jacob Carlborg
Aug 17 2011
prev sibling parent jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 15:54, kennytm skrev:

 Jonas Drewsen<jdrewsen nospam.com>  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


 1) Why 'Http' and 'Ftp' but 'SMTP' is in all caps?


Will fix


 2) Some links in the doc says 'std.curl....'


I know. This is a limitation of the ddoc config file. I guess I have to 
make a patch for that one as well.

Thanks
Jonas
Aug 16 2011
prev sibling next sibling parent reply Alix Pexton <alix.DOT.pexton gmail.DOT.com> writes:
On 16/08/2011 12:48, 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


Spotted another issue...

In the example for Protocol.onReceive, there is a mismatch between the 
parameter names specified for the delegate and those used inside it.

Also, all the XREF macros point to std_curl.html. instead of 
etc_curl.html, is this going to become std.curl when incorporated into 
Phobos?

A...
Aug 16 2011
parent reply jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 16:43, Alix Pexton skrev:

 On 16/08/2011 12:48, 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


 Spotted another issue...

 In the example for Protocol.onReceive, there is a mismatch between the
 parameter names specified for the delegate and those used inside it.


you mean this one:

client.onReceive = (ubyte[] data) { writeln("Got data", cast(char[]) 
data); return data.length;};

I cannot identify the problem?


 Also, all the XREF macros point to std_curl.html. instead of
 etc_curl.html, is this going to become std.curl when incorporated into
 Phobos?

 A...


A fix to the std ddoc config is needed.

As explained in reply to another post I believe this wrapper should be 
put in the etc package.

/Jonas
Aug 16 2011
parent reply Alix Pexton <alix.DOT.pexton gmail.DOT.com> writes:
On 16/08/2011 20:55, jdrewsen wrote:

 Den 16-08-2011 16:43, Alix Pexton skrev:

 On 16/08/2011 12:48, 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


 Spotted another issue...

 In the example for Protocol.onReceive, there is a mismatch between the
 parameter names specified for the delegate and those used inside it.


 you mean this one:

 client.onReceive = (ubyte[] data) { writeln("Got data", cast(char[])
 data); return data.length;};

 I cannot identify the problem?


My mistake, I was in a hurry and mixed up the functions ><
This is the snippet in question (lines 414-417 in the source)


 http.onProgress = delegate int(double dl, double dln, double ul, double ult) {
 writeln("Progress: downloaded ", dln, " of ", dl);
 writeln("Progress: uploaded ", uln, " of ", ul);
        };


The last parameter is named "ult", but the writeln is passed "uln", I 
think more descriptive names might help.

A...
Aug 16 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 17-08-2011 00:17, Alix Pexton skrev:

 On 16/08/2011 20:55, jdrewsen wrote:

 Den 16-08-2011 16:43, Alix Pexton skrev:

 On 16/08/2011 12:48, 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


 Spotted another issue...

 In the example for Protocol.onReceive, there is a mismatch between the
 parameter names specified for the delegate and those used inside it.


 you mean this one:

 client.onReceive = (ubyte[] data) { writeln("Got data", cast(char[])
 data); return data.length;};

 I cannot identify the problem?


 My mistake, I was in a hurry and mixed up the functions ><
 This is the snippet in question (lines 414-417 in the source)


 http.onProgress = delegate int(double dl, double dln, double ul,
 double ult) {
 writeln("Progress: downloaded ", dln, " of ", dl);
 writeln("Progress: uploaded ", uln, " of ", ul);
 };


 The last parameter is named "ult", but the writeln is passed "uln", I
 think more descriptive names might help.

 A...


ok

Thanks
Jonas
Aug 17 2011
prev sibling next sibling parent reply David Nadlinger <see klickverbot.at> writes:
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.

David


On 8/16/11 1:48 PM, 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
Aug 16 2011
next sibling parent reply "Nick Sabalausky" <a a.a> writes:
"David Nadlinger" <see klickverbot.at> wrote in message 
news:j2e15a$2nsh$1 digitalmars.com...

 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.


Curious: What exactly is the role of the review manager?
Aug 16 2011
parent reply "Lars T. Kyllingstad" <public kyllingen.NOSPAMnet> writes:
On Tue, 16 Aug 2011 12:26:19 -0400, Nick Sabalausky wrote:


 "David Nadlinger" <see klickverbot.at> wrote in message
 news:j2e15a$2nsh$1 digitalmars.com...

 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.


 Curious: What exactly is the role of the review manager?


Andrei has suggested we follow Boost's review model:

http://www.boost.org/community/reviews.html

-Lars
Aug 16 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 19:15, Lars T. Kyllingstad skrev:

 On Tue, 16 Aug 2011 12:26:19 -0400, Nick Sabalausky wrote:


 "David Nadlinger"<see klickverbot.at>  wrote in message
 news:j2e15a$2nsh$1 digitalmars.com...

 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.


 Curious: What exactly is the role of the review manager?


 Andrei has suggested we follow Boost's review model:

 http://www.boost.org/community/reviews.html

 -Lars


And I see that the link contains info for the library developer as well. 
Have to read.

/Jonas
Aug 16 2011
prev sibling next sibling parent reply Jimmy Cao <jcao219 gmail.com> writes:
On Tue, Aug 16, 2011 at 10:08 AM, David Nadlinger <see klickverbot.at>wrote=
:


 Seems like the reviews are already coming in steadily, but we still have =


no

 review manager =96 if nobody else steps up until tomorrow, I'd volunteer =


so we

 can get the formal review process going.

 David



 On 8/16/11 1:48 PM, 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<https://git=




hub.com/jcd/phobos/blob/curl-wrapper/etc/curl.d>

 Docs:
 http://freeze.steamwinter.com/**D/web/phobos/etc_curl.html<http://freeze=




.steamwinter.com/D/web/phobos/etc_curl.html>

 Demolish!

 /Jonas




On line 80, I made a typo when writing the documentation:

https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L80

It should be:
<to.addr gmail.com>

After reading the libcurl documentation, I realize that the automatic angle
bracket attaching at lines 2932 and 2943 are not needed (libcurl seems to d=
o
it for you after version 7.21.4).
Aug 16 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 16-08-2011 19:05, Jimmy Cao skrev:

 On Tue, Aug 16, 2011 at 10:08 AM, David Nadlinger <see klickverbot.at
 <mailto:see klickverbot.at>> 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.

     David



     On 8/16/11 1:48 PM, 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
         <https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d>
         Docs:
         http://freeze.steamwinter.com/__D/web/phobos/etc_curl.html
         <http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>

         Demolish!

         /Jonas




 On line 80, I made a typo when writing the documentation:

 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L80

 It should be:
 <to.addr gmail.com <mailto:to.addr gmail.com>>


ok


 After reading the libcurl documentation, I realize that the automatic
 angle bracket attaching at lines 2932 and 2943 are not needed (libcurl
 seems to do it for you after version 7.21.4).


Will fix

Thanks
Jonas
Aug 16 2011
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, August 16, 2011 17:08:26 David Nadlinger wrote:

 Seems like the reviews are already coming in steadily, but we still h=


ave

 no review manager =E2=80=93 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=20
response, so std.path got reviewed first. So, I can do it, but it's fin=
e with me=20
if you do it. I only volunteered before in attempt to get it moving, wh=
ich=20
didn't happen at the time.

- Jonathan MDavis
Aug 16 2011
prev sibling next sibling parent reply dsimcha <dsimcha yahoo.com> writes:
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


One more comment:  In various places, you have someFunction(in 
const(char)[] someString).  Const and in are redundant.  In is 
synonymous with const.

Also, anyone have any luck getting Curl libs to work with DMD on 
Windows?  The binaries available at http://curl.haxx.se/download.html 
look like they're COFF.  (<rant> Why do we still use freakin' OMF? 
</rant>)  Also, the source doesn't look like it's pleasant to compile.
Aug 17 2011
parent Jimmy Cao <jcao219 gmail.com> writes:
On Wed, Aug 17, 2011 at 10:41 PM, 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<https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d>
 Docs:
 http://freeze.steamwinter.com/**D/web/phobos/etc_curl.html<http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>

 Demolish!

 /Jonas


 One more comment:  In various places, you have someFunction(in
 const(char)[] someString).  Const and in are redundant.  In is synonymous
 with const.

 Also, anyone have any luck getting Curl libs to work with DMD on Windows?
  The binaries available at
http://curl.haxx.se/download.**html<http://curl.haxx.se/download.html>look like
they're COFF.  (<rant> Why do we still use freakin' OMF? </rant>)
  Also, the source doesn't look like it's pleasant to compile.


I've always been using etc.c.curl on Windows.  Use implib to generate .lib
files from the dlls.
Aug 17 2011
prev sibling next sibling parent reply "Martin Nowak" <dawg dawgfoto.de> writes:
On Tue, 16 Aug 2011 13:48:57 +0200, Jonas Drewsen <jdrewsen nospam.com>  
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


Just want to make sure these two code comments don't get lost.

https://github.com/jcd/phobos/commit/9177203c1e54c4be959fb0b81e9d84f3d5e861f9#etc/curl.d-P132
https://github.com/jcd/phobos/commit/9177203c1e54c4be959fb0b81e9d84f3d5e861f9#etc/curl.d-P192

martin
Aug 18 2011
parent jdrewsen <jdrewsen nospam.com> writes:
Den 18-08-2011 20:12, Martin Nowak skrev:

 On Tue, 16 Aug 2011 13:48:57 +0200, Jonas Drewsen <jdrewsen nospam.com>
 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


 Just want to make sure these two code comments don't get lost.

 https://github.com/jcd/phobos/commit/9177203c1e54c4be959fb0b81e9d84f3d5e861f9#etc/curl.d-P132

 https://github.com/jcd/phobos/commit/9177203c1e54c4be959fb0b81e9d84f3d5e861f9#etc/curl.d-P192


 martin


I did see them.

Thanks
/Jonas
Aug 19 2011
prev sibling next sibling parent Long Chang <changedalone gmail.com> writes:
http://gool.googlecode.com/files/libcurl_7.21.7.zip
this libcurl static lib is build by dmc , with openssl support .

On 16 August 2011 19:48, Jonas Drewsen <jdrewsen nospam.com> 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<https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d>
 Docs:
        http://freeze.steamwinter.com/**D/web/phobos/etc_curl.html<http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>

 Demolish!

 /Jonas
Aug 18 2011
prev sibling parent reply dav1d <admin dav1d.de> writes:
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


Hello,

At first a little story, to skip it just begin reading after the paragraph.
I'm still a D "beginner", I set myself as goal translating an already written
Python lib into D, which isn't easy with broken "std.json" etc. so I was quite
happy when I found etc.curl (#curl on freenode linked me there). Well I was a
bit surprised when I saw Http is a struct, anyway it works and looks good. So
while I was working I encountered that the website I sent the requests to
always gave me error codes, I was wondering why, I set _all_ headers correctly
(checked it twice) (Yeah I've to set a "Cookie" Header...) and the requests
also looked correct .. so I started Wireshark sniffing what actually is beeing
sent .. I realized: not a single Header is sent, WTF, so I took another look in
the code and I went crazy ..

Setting headers to the Http struct works .. but if you call .post a Result
struct is created and just one Header "Content-Type" is passed to it and then
you .postData gets called on the Result struct. (look at the name .. it should
contain the result and not send the request). This behavour is undocumented and
totally ridiculous. 

I'm coming from Python, Pythons urrlib hasn't the best api (well some might say
the api sucks) but etc.curl is on top of that.

My personal opinion is rewrite this module or dont put it in the std.lib, if
you do, this will confuse and drive lots of people crazy.
Aug 24 2011
parent reply Johannes Pfau <spam example.com> writes:
dav1d wrote:

Jonas Drewsen Wrote:


 Hi all,
=20
     This is a review request for the curl wrapper. Please read the=20
 "known issues" in the top of the source file and if possible suggest
 a solution.
=20
 We also need somebody for running the review process. Anyone?
=20
 Code:
 	https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d=09
 Docs:
 	http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
=20
 Demolish!
=20
 /Jonas


Hello,

At first a little story, to skip it just begin reading after the
paragraph. I'm still a D "beginner", I set myself as goal translating
an already written Python lib into D, which isn't easy with broken
"std.json" etc. so I was quite happy when I found etc.curl (#curl on
freenode linked me there). Well I was a bit surprised when I saw Http
is a struct, anyway it works and looks good. So while I was working I
encountered that the website I sent the requests to always gave me
error codes, I was wondering why, I set _all_ headers correctly
(checked it twice) (Yeah I've to set a "Cookie" Header...) and the
requests also looked correct .. so I started Wireshark sniffing what
actually is beeing sent .. I realized: not a single Header is sent,
WTF, so I took another look in the code and I went crazy ..

Setting headers to the Http struct works .. but if you call .post a
Result struct is created and just one Header "Content-Type" is passed
to it and then you .postData gets called on the Result struct. (look
at the name .. it should contain the result and not send the request).
This behavour is undocumented and totally ridiculous.=20

I'm coming from Python, Pythons urrlib hasn't the best api (well some
might say the api sucks) but etc.curl is on top of that.

My personal opinion is rewrite this module or dont put it in the
std.lib, if you do, this will confuse and drive lots of people crazy.


Well, your problem is caused by two things: The API docs are not very
detailed here and D allows static methods to be called on
instances, which can be confusing.

The post method is a static method, so it can't use headers you've set
with addHeader/setHeader. If you use an Http instance like this:

Http client =3D Http("http://google.com");
client.addHeader("Test", "Header");

you _must not_ call client.post()
instead you have to set the method property and use perform:

client.method =3D Http.Method.post; //AFAIK post is default, so you can
                                  //skip this line
client.perform();

The post/get/put methods are only convenience methods with very limited
functionality. They only exist to make simple requests with one line of
code.

--=20
Johannes Pfau
Aug 24 2011
parent reply jdrewsen <jdrewsen nospam.com> writes:
Den 24-08-2011 13:04, Johannes Pfau skrev:

 dav1d wrote:

 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


 Hello,

 At first a little story, to skip it just begin reading after the
 paragraph. I'm still a D "beginner", I set myself as goal translating
 an already written Python lib into D, which isn't easy with broken
 "std.json" etc. so I was quite happy when I found etc.curl (#curl on
 freenode linked me there). Well I was a bit surprised when I saw Http
 is a struct, anyway it works and looks good. So while I was working I
 encountered that the website I sent the requests to always gave me
 error codes, I was wondering why, I set _all_ headers correctly
 (checked it twice) (Yeah I've to set a "Cookie" Header...) and the
 requests also looked correct .. so I started Wireshark sniffing what
 actually is beeing sent .. I realized: not a single Header is sent,
 WTF, so I took another look in the code and I went crazy ..

 Setting headers to the Http struct works .. but if you call .post a
 Result struct is created and just one Header "Content-Type" is passed
 to it and then you .postData gets called on the Result struct. (look
 at the name .. it should contain the result and not send the request).
 This behavour is undocumented and totally ridiculous.

 I'm coming from Python, Pythons urrlib hasn't the best api (well some
 might say the api sucks) but etc.curl is on top of that.

 My personal opinion is rewrite this module or dont put it in the
 std.lib, if you do, this will confuse and drive lots of people crazy.


 Well, your problem is caused by two things: The API docs are not very
 detailed here and D allows static methods to be called on
 instances, which can be confusing.

 The post method is a static method, so it can't use headers you've set
 with addHeader/setHeader. If you use an Http instance like this:

 Http client = Http("http://google.com");
 client.addHeader("Test", "Header");

 you _must not_ call client.post()
 instead you have to set the method property and use perform:

 client.method = Http.Method.post; //AFAIK post is default, so you can
                                    //skip this line
 client.perform();

 The post/get/put methods are only convenience methods with very limited
 functionality. They only exist to make simple requests with one line of
 code.


One way that may improve this would be to move the static methods 
outside the class and make them into module functions instead.

The drawbacks of this is:

1, When importing the module there would be more symbols polluting the 
namespace.

2, We would have to rename from Http.get to a function named HttpGet. We 
cannot call it simply "get" because both Http and Ftp has the get method.

And personally i lige the Http.get syntax better.

Any other suggestions?

/Jonas
Aug 24 2011
next sibling parent Adam Ruppe <destructionator gmail.com> writes:
For what it's worth, my little curl.d module has one magic
function that just kinda does it all:

string curl(string url, string postData = null, string postDataContentType =
"application/x-www-form-data"); (or whatever the content type is)

This does simple stuff, then there's other ways to do fancier
things.

string site = curl("http://mysite.com/"); // fetch the html

curl("http://mysite.com/item", "a=20&b=30"); // does a POST


Adding basic FTP get/put based on the url might be a good one too.
Aug 24 2011
prev sibling next sibling parent dsimcha <dsimcha yahoo.com> writes:
== Quote from jdrewsen (jdrewsen nospam.com)'s article

 One way that may improve this would be to move the static methods
 outside the class and make them into module functions instead.
 The drawbacks of this is:
 1, When importing the module there would be more symbols polluting the
 namespace.


I don't consider this much of a problem.  If you want to use module that
pollutes
the namespace a lot with rarely called functions that have short, common names,
that's what static or local (i.e. within a function/class/struct, which only can
be done since the last release of DMD) imports are for.  It's a problem in other
languages, but in D there are elegant solutions.

As an example, before I knew about static imports and before we had local
imports,
std.file's propensity to collide with just about everything due to its use of
short, common names drove me crazy.  Now, I really don't mind because I always
import it either statically or locally.
Aug 24 2011
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-08-24 20:36, jdrewsen wrote:

 One way that may improve this would be to move the static methods
 outside the class and make them into module functions instead.

 The drawbacks of this is:

 1, When importing the module there would be more symbols polluting the
 namespace.

 2, We would have to rename from Http.get to a function named HttpGet. We
 cannot call it simply "get" because both Http and Ftp has the get method.

 And personally i lige the Http.get syntax better.

 Any other suggestions?

 /Jonas


You could wrap the free functions in a Curl struct:

Curl.get(...);

-- 
/Jacob Carlborg
Aug 24 2011
parent reply David Nadlinger <see klickverbot.at> writes:
On 8/25/11 8:53 AM, Jacob Carlborg wrote:

 On 2011-08-24 20:36, jdrewsen wrote:

 One way that may improve this would be to move the static methods
 outside the class and make them into module functions instead.

 The drawbacks of this is:

 1, When importing the module there would be more symbols polluting the
 namespace.

 […]


 You could wrap the free functions in a Curl struct:

 Curl.get(...);


Please don't (ab)use classes/structs as namespace if you don't have a 
really good reason to do so – D offers static and/or selective imports 
for situation where one doesn't want to pollute the scope when importing 
something. When using free functions, one can always do »static import 
etc.curl;«, but there is no way back once a dummy struct is used (well, 
technically I think it would be possible to write a mixin template that 
aliases all struct members into the local scope, but…).

David
Aug 31 2011
parent Ary Manzana <ary esperanto.org.ar> writes:
On 8/31/11 11:29 PM, David Nadlinger wrote:

 On 8/25/11 8:53 AM, Jacob Carlborg wrote:

 On 2011-08-24 20:36, jdrewsen wrote:

 One way that may improve this would be to move the static methods
 outside the class and make them into module functions instead.

 The drawbacks of this is:

 1, When importing the module there would be more symbols polluting the
 namespace.

 […]


 You could wrap the free functions in a Curl struct:

 Curl.get(...);


 Please don't (ab)use classes/structs as namespace if you don't have a
 really good reason to do so – D offers static and/or selective imports
 for situation where one doesn't want to pollute the scope when importing
 something. When using free functions, one can always do »static import
 etc.curl;«, but there is no way back once a dummy struct is used (well,
 technically I think it would be possible to write a mixin template that
 aliases all struct members into the local scope, but…).

 David


Polluting the global namespace: it's called "dealing with the language" 
instead of "dealing with the problem you want to solve".
Aug 31 2011