Curl wrapper review

[Jonas Drewsen]

On 16/11/11 20.21, Johannes Pfau wrote:
> Jonas Drewsen wrote:
>> Hi,
>>
>>     After all the comments from last review I've refactored the curl
>> wrapper and it is ready for a new review.
>>
>> David Nadlinger was handling the last review so I guess it would make
>> sense if he run this one as well if he wants to.
>>
>> Code:
>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
>>
>> Docs:
>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>
>> Regards,
>> Jonas
>
> Looks great so far!
>
> Some minor nitpicks:
>
> * Examples: "new Http()" is used 4 times in the examples, but as Http is
>    a struct, I see no reason to create it using new?

new Http() calls opCall() where a bunch of stuff is initialized.


> * Move
>    ----------------------------------------------
>    // Get using an line range and proxy settings
>    auto client = new Http();
>    client.proxy = "1.2.3.4";
>    foreach (line; byLine("d-p-l.org", client))
>        writeln(line);
>    ----------------------------------------------
>    from the first to the second example. (The sentence "For more control
>    than the high level functions provide, use the low level API:" sounds
>    like everything shown up to that point was the high level api, so that
>    one example should be moved below that sentence)

It is stille the high level API. It is just an example of how to set 
special options using the high level API.

The low level API is the one that requires you to register callbacks and 
convert data yourself etc.

> * smtp.perform; -->  smtp.perform();

ok

> * Please set default timeouts for the simple API. Nothing is more
>    annoying than a application locking up because of internet loss..

ok

> * Speaking of timeouts, I'd like a special TimeoutException, as it's
>    easy to recover from these errors.

ok

> * At least the high-level API should really be @safe (or @trusted).
>    Adding those qualifiers to the low level API would also be good, but
>    that can always be done after merging into phobos.

I looked at using safe and trusted as well but skipped it - maybe 
someone can tell me why this is a good thing to do. I do understand the 
value of tagging functions with @safe, at trusted and @system. What I fail 
to see is why the hole concept is not degraded when @safe is allowed to 
call @trusted.

A programmer would look at the @safe function definition and think it is 
safe and it doesn't do weird casts etc. But this is misleading when 
@trusted functions are called from inside the @safe function.

Shouldn't @trusted taint @safe functions and make the compiler complain 
when @trusted is called from within a @safe function.

I _have_ read the specs. and do know that this is how it is supposed to 
work. I just need some reasoning behind it.


> * Some functions could be nothrow? @property StatusLine statusLine()
>    for example, maybe some more.

I went through all functions to check for nothrow but since RefCounted 
in almost all functions and methods on the RefCounted is not nothrow I 
cannot mark anything as nothrow myself.

Thanks
/Jonas