Curl wrapper review

[Johannes Pfau]

Jonas Drewsen wrote:
>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.

I'm not complaining about the initialization, but about using 'new'.
______________________
auto client = Http();
----------------------
will call the opCall (and you also have this example in the docs), but
it'll allocate client on the stack, where new will use the GC to
allocate it on the heap.

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

OK I'd agree with that, but the cheat sheet clearly says:
* High level:
  download,upload,get,put,byLine,byChunk,byLineAsync,byChunkAsync
* Low level:
  Http, Ftp, Smtp

so the docs are a little inconsistent in that regard.

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

@safe tells the compiler: This function only uses the safeD subset,
complain if I do unsafe stuff.

@trusted means: This function can do anything, but the programmer
guarantees that it's safe to call this function from safeD. This means
it's not possible to pass values to the function which can undermine
the safeD guarantees:

//NOT trusted! (This method breaks if a invalid pointer is passed)
void noTrust(int* ptr)
{
    *ptr = 0;
}


int value;
@trusted void trustMe(int* ptr)
{
    //Do something to verify input
    if(ptr != &value)
        return;

    *ptr = 0;
}

So there should be few @trusted code and it should be well tested, but
it is necessary to have some sort of 'bridge' between safe and unsafe
code. To put it short, @trusted allows you to write wrapper functions
for @system code.

-- 
Johannes Pfau