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 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 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 next sibling 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 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 next 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 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 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 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>






 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

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

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

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

 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 "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
prev 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 "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
prev sibling parent "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
prev sibling next 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?

 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?

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

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?

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?

 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?

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

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?

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

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

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
parent 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
prev sibling next sibling 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 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

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 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 next sibling parent 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
 

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
prev 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 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 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 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 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.


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:
--0015173feec43350f604aaa265b2
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

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 =

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

 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=


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


 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). --0015173feec43350f604aaa265b2 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On Tue, Aug 16, 2011 at 10:08 AM, David Nadlinger <span dir=3D"ltr">&lt;<a = href=3D"mailto:see klickverbot.at" target=3D"_blank">see klickverbot.at</a>= &gt;</span> wrote:<br><div class=3D"gmail_quote"><blockquote class=3D"gmail= _quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:= 1ex"> 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&#39;d volunte= er so we can get the formal review process going.<br><font color=3D"#888888= "> <br> David</font><div><div></div><div><br> <br> <br> On 8/16/11 1:48 PM, Jonas Drewsen wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"> Hi all,<br> <br> This is a review request for the curl wrapper. Please read the &quot;known<= br> issues&quot; in the top of the source file and if possible suggest a soluti= on.<br> <br> We also need somebody for running the review process. Anyone?<br> <br> Code:<br> <a href=3D"https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d" targ= et=3D"_blank">https://github.com/jcd/phobos/<u></u>blob/curl-wrapper/etc/cu= rl.d</a><br> Docs:<br> <a href=3D"http://freeze.steamwinter.com/D/web/phobos/etc_curl.html" target= =3D"_blank">http://freeze.steamwinter.com/<u></u>D/web/phobos/etc_curl.html= </a><br> <br> Demolish!<br> <br> /Jonas<br> </blockquote> <br> </div></div></blockquote></div><br><div><br></div><div>On line 80, I made a= typo when writing the documentation:</div><div><br></div><div><a href=3D"h= ttps://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L80" target=3D"_b= lank">https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L80</a></d= iv> <div><br></div><div>It should be:</div>&lt;<a href=3D"mailto:to.addr gmail.= com" target=3D"_blank">to.addr gmail.com</a>&gt;<div><br></div><div>After r= eading the libcurl documentation, I realize that the automatic angle bracke= t attaching at lines 2932 and 2943 are not needed (libcurl seems to do it f= or you after version 7.21.4).</div> <div><br></div> --0015173feec43350f604aaa265b2--
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 next sibling parent "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.


Andrei has suggested we follow Boost's review model: http://www.boost.org/community/reviews.html -Lars
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=

 no review manager =E2=80=93 if nobody else steps up until tomorrow, I=

 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 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
prev sibling next sibling parent 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
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:
--001517477f20cac65a04aac00b6a
Content-Type: text/plain; charset=ISO-8859-1

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. --001517477f20cac65a04aac00b6a Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On Wed, Aug 17, 2011 at 10:41 PM, dsimcha <span dir=3D"ltr">&lt;<a href=3D"= mailto:dsimcha yahoo.com">dsimcha yahoo.com</a>&gt;</span> wrote:<br><div c= lass=3D"gmail_quote"><blockquote class=3D"gmail_quote" style=3D"margin:0 0 = 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> <div class=3D"im">On 8/16/2011 7:48 AM, Jonas Drewsen wrote:<br> </div><div><div></div><div class=3D"h5"><blockquote class=3D"gmail_quote" s= tyle=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi all,<br> <br> This is a review request for the curl wrapper. Please read the &quot;known<= br> issues&quot; in the top of the source file and if possible suggest a soluti= on.<br> <br> We also need somebody for running the review process. Anyone?<br> <br> Code:<br> <a href=3D"https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d" targ= et=3D"_blank">https://github.com/jcd/phobos/<u></u>blob/curl-wrapper/etc/cu= rl.d</a><br> Docs:<br> <a href=3D"http://freeze.steamwinter.com/D/web/phobos/etc_curl.html" target= =3D"_blank">http://freeze.steamwinter.com/<u></u>D/web/phobos/etc_curl.html= </a><br> <br> Demolish!<br> <br> /Jonas<br> </blockquote> <br></div></div> One more comment: =A0In various places, you have someFunction(in const(char= )[] someString). =A0Const and in are redundant. =A0In is synonymous with co= nst.<br> <br> Also, anyone have any luck getting Curl libs to work with DMD on Windows? = =A0The binaries available at <a href=3D"http://curl.haxx.se/download.html" = target=3D"_blank">http://curl.haxx.se/download.<u></u>html</a> look like th= ey&#39;re COFF. =A0(&lt;rant&gt; Why do we still use freakin&#39; OMF? &lt;= /rant&gt;) =A0Also, the source doesn&#39;t look like it&#39;s pleasant to c= ompile.<br> </blockquote><div><br></div><div>I&#39;ve always been using etc.c.curl on W= indows. =A0Use implib to generate .lib files from the dlls.=A0</div></div><= br> --001517477f20cac65a04aac00b6a--
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 "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> 






 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
 

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
prev sibling next sibling parent Long Chang <changedalone gmail.com> writes:
--bcaec51b9de33aa63c04aad2efa7
Content-Type: text/plain; charset=UTF-8

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

--bcaec51b9de33aa63c04aad2efa7 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable <a href=3D"http://gool.googlecode.com/files/libcurl_7.21.7.zip">http://gool= .googlecode.com/files/libcurl_7.21.7.zip</a><div>this libcurl static lib is= build by dmc , with openssl support .<br><br><div class=3D"gmail_quote">On= 16 August 2011 19:48, Jonas Drewsen <span dir=3D"ltr">&lt;<a href=3D"mailt= o:jdrewsen nospam.com">jdrewsen nospam.com</a>&gt;</span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex;">Hi all,<br> <br> =C2=A0 This is a review request for the curl wrapper. Please read the &quo= t;known issues&quot; in the top of the source file and if possible suggest = a solution.<br> <br> We also need somebody for running the review process. Anyone?<br> <br> Code:<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0<a href=3D"https://github.com/jcd/phobos/blob/c= url-wrapper/etc/curl.d" target=3D"_blank">https://github.com/jcd/phobos/<u>= </u>blob/curl-wrapper/etc/curl.d</a> =C2=A0 =C2=A0 =C2=A0<br> Docs:<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0<a href=3D"http://freeze.steamwinter.com/D/web/= phobos/etc_curl.html" target=3D"_blank">http://freeze.steamwinter.com/<u></= u>D/web/phobos/etc_curl.html</a><br> <br> Demolish!<br><font color=3D"#888888"> <br> /Jonas<br> </font></blockquote></div><br></div> --bcaec51b9de33aa63c04aad2efa7--
Aug 18 2011
prev sibling next sibling parent "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
 

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
prev sibling next sibling parent "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
 

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
prev sibling next 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 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.

 […]

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.

 […]

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
prev sibling parent Johannes Pfau <spam example.com> writes:
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

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