Em 22/06/2011, às 07:02, Jonas Drewsen <jdrewsen@nospam.com> escreveu:

> On 22/06/11 06.11, Jose Armando Garcia wrote:
>>>> Hi Jonas,
>>>>
>>>> Was reading your implementation but I had to context switch. Only go
>>>> to line 145 :(. I see that you are refcounting by sharing a uint* but
>>>> what about all the other private fields? What happens if you pass the
>>>> Curl object around functions and those values are modified?
>>>>
>>>> Thanks,
>>>> -Jose
>>>
>>> The refcounting is actually just needed to keep the "handle" alive under
>>> construction of the Curl object using "Curl()". I'm using "Curl()" by
>>> defining opCall on Curl in order not to have a struct constructor with a
>>> dummy parameter (structs cannot have a default constructor defined).
>>>
>>> After that the Curl instance will always be assigned to a member variable of
>>> Http/Ftp classes. Instances of Http/Ftp are not to be copied because they
>>> are used for RAII.
>>>
>> Http/Ftp are structs not classes. Let me try to understand this. You
>> mean to say that the Http and Ftp struct are not to be passed to other
>> functions? Are you expecting the user to do all their IO in one scope?
>> This is unnecessarily limiting.
>>
>
> They are structs because they use a Curl instance which in turn uses RAII in order not to leak curl handles. If they were classes it would be easy to leak because you never know when the GC is coming around to call the destructor and release the curl handle.
>
> Your can pass Http/Ftp structs to other function either by givin them a reference to a local instance or you could simply do a:
>
> auto http = new Http("http://google.com");
> passItOn(http);
>
> /Jonas

Passing refs around doesn't work if the function wants to store it on the heap for later.

Have you taken a look at how std.stdio.File and std.typecons.RefCounted do reference counting?

Den 22-06-2011 10:25, Johannes Pfau skrev:
> jdrewsen wrote:
>> Den 21-06-2011 12:55, Johannes Pfau skrev:
>>> jdrewsen wrote:
>>>> Den 18-06-2011 22:36, jdrewsen skrev:
>>>>> Hi,
>>>>>
>>>>> I've finally got through all the very constructive comments from
>>>>> the last review of the curl wrapper and performed the needed
>>>>> changes.
>>>>>
>>>>> Here is the github branch:
>>>>> https://github.com/jcd/phobos/tree/curl-wrapper
>>>>>
>>>>> And the generated docs:
>>>>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>>
>>>> I've made the changes as suggested from your comments and pushed to
>>>> the github branch above.
>>>>
>>>> Changes:
>>>>
>>>> * Change and delete individual headers when using static convenience
>>>> methods
>>>> * Make keep-alive work when using static convenience methods
>>>> * Add as extra modifiable parameters on follow requests
>>>> (keep-alive): headers, method, url, postData
>>>> * Add verbose property to Protocol
>>>> * No dummy bool in constructors
>>>>
>>>> Comments are welcome
>>>>
>>>> /Jonas
>>>
>>> Oh, and btw are you reading the phobos-dev mailing list? Seems like
>>> there are no code reviews planned right now, so if you think etc.curl
>>> was ready for review, you could propose it now?
>>
>> It don't know anything about a phobos-dev mailing list. The only
>> reviews I've seen so far is on this D newsgroup. How do I get on that
>> mailing list?
>
> Here's the web interface:
> http://lists.puremagic.com/mailman/listinfo (the 'phobos' list)
> Reviews are always done on the newsgroup, but some phobos related
> discussion is also happening on the mailing list.

Thanks!

>
>> I'll think It'll soon be ready for an official code review though.
>>
>> /Jonas
>>

Den 22-06-2011 17:34, Jose Armando Garcia skrev:
> Em 22/06/2011, às 07:02, Jonas Drewsen <jdrewsen@nospam.com> escreveu:
>
>> On 22/06/11 06.11, Jose Armando Garcia wrote:
>>>>> Hi Jonas,
>>>>>
>>>>> Was reading your implementation but I had to context switch. Only go
>>>>> to line 145 :(. I see that you are refcounting by sharing a uint* but
>>>>> what about all the other private fields? What happens if you pass the
>>>>> Curl object around functions and those values are modified?
>>>>>
>>>>> Thanks,
>>>>> -Jose
>>>>
>>>> The refcounting is actually just needed to keep the "handle" alive
>>>> under
>>>> construction of the Curl object using "Curl()". I'm using "Curl()" by
>>>> defining opCall on Curl in order not to have a struct constructor
>>>> with a
>>>> dummy parameter (structs cannot have a default constructor defined).
>>>>
>>>> After that the Curl instance will always be assigned to a member
>>>> variable of
>>>> Http/Ftp classes. Instances of Http/Ftp are not to be copied because
>>>> they
>>>> are used for RAII.
>>>>
>>> Http/Ftp are structs not classes. Let me try to understand this. You
>>> mean to say that the Http and Ftp struct are not to be passed to other
>>> functions? Are you expecting the user to do all their IO in one scope?
>>> This is unnecessarily limiting.
>>>
>>
>> They are structs because they use a Curl instance which in turn uses
>> RAII in order not to leak curl handles. If they were classes it would
>> be easy to leak because you never know when the GC is coming around to
>> call the destructor and release the curl handle.
>>
>> Your can pass Http/Ftp structs to other function either by givin them
>> a reference to a local instance or you could simply do a:
>>
>> auto http = new Http("http://google.com");
>> passItOn(http);
>>
>> /Jonas
>
> Passing refs around doesn't work if the function wants to store it on
> the heap for later.
>
> Have you taken a look at how std.stdio.File and std.typecons.RefCounted
> do reference counting?

I think I'll change it to match std.stdio.File in the name of consistency.

Thanks
/Jonas

Den 22-06-2011 12:08, Dmitry Olshansky skrev:
> On 22.06.2011 14:02, Jonas Drewsen wrote:
>> On 22/06/11 06.11, Jose Armando Garcia wrote:
>>>>> Hi Jonas,
>>>>>
>>>>> Was reading your implementation but I had to context switch. Only go
>>>>> to line 145 :(. I see that you are refcounting by sharing a uint* but
>>>>> what about all the other private fields? What happens if you pass the
>>>>> Curl object around functions and those values are modified?
>>>>>
>>>>> Thanks,
>>>>> -Jose
>>>>
>>>> The refcounting is actually just needed to keep the "handle" alive
>>>> under
>>>> construction of the Curl object using "Curl()". I'm using "Curl()" by
>>>> defining opCall on Curl in order not to have a struct constructor
>>>> with a
>>>> dummy parameter (structs cannot have a default constructor defined).
>>>>
>>>> After that the Curl instance will always be assigned to a member
>>>> variable of
>>>> Http/Ftp classes. Instances of Http/Ftp are not to be copied because
>>>> they
>>>> are used for RAII.
>>>>
>>> Http/Ftp are structs not classes. Let me try to understand this. You
>>> mean to say that the Http and Ftp struct are not to be passed to other
>>> functions? Are you expecting the user to do all their IO in one scope?
>>> This is unnecessarily limiting.
>>>
>>
>> They are structs because they use a Curl instance which in turn uses
>> RAII in order not to leak curl handles. If they were classes it would
>> be easy to leak because you never know when the GC is coming around to
>> call the destructor and release the curl handle.
>>
>> Your can pass Http/Ftp structs to other function either by givin them
>> a reference to a local instance or you could simply do a:
>>
>> auto http = new Http("http://google.com");
>
> I suspect that http is on _heap_ just like class, and it's destructor is
> never called ATM. Even when that's fixed it's still non-deterministic.
> Maybe refcounting? e.g. std.typecons.RefCounted
>
>> passItOn(http);
>>
>> /Jonas

As I replied a minute ago to Jose I'll change it to match the std.stdio.File behavior.

/Jonas