Curl wrapper review

[Jonas Drewsen]

On 17/11/11 12.42, Johannes Pfau wrote:
> 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.

Thats how I understand it as well. My issue is that @safe attribute on a 
function really degrades to @trusted if the function calls a @trusted 
function. This means that a programmer that sees a @safe attribute on a 
function really can't rely on the safe properties of calling it. But 
this is only a documentation issue I guess.

Anyway I'll put @safe and @trusted attr. on the appropriate functions.

/Jonas