Second Round CURL Wrapper Review

[jdrewsen]

On Sunday, 18 December 2011 at 01:27:45 UTC, Jonathan M Davis 
wrote:
> On Saturday, December 17, 2011 23:10:00 jdrewsen wrote:
>> On Thursday, 15 December 2011 at 08:51:29 UTC, Jonathan M Davis
>> 
>> wrote:
>> > Line# 235 is identical to line# 239. Shouldn't line# 235 be
>> > creating an Http object, not an Ftp object? That mistake
>> > definitely makes it look like download hasn't been properly
>> > tested.
>> 
>> It has been tested as you can see by the unittest just below 
>> the
>> function. It just did not fail because libcurl requires the 
>> same
>> setup for this download situation for both ftp and http. Will 
>> fix
>> it of course.
>
> Well, when there's code like that that is clearly wrong, it 
> makes it look like it's either not being tested by the unit 
> tests or that the unit tests haven't been run. I guess that 
> this is just a case where the unit tests are unable to catch 
> the problem.
>
>> > Is there a reason that the functions which have a template
>> > argument T which can be either a char or a ubyte can't work
>> > with immutable char? Glancing at _basicHttp, I don't see any
>> > reason why T couldn't be immutable char. Yes, it would 
>> > require
>> > casting to immutable(char)[], but you're already casting to
>> > char[], and the data being returned appears to be unique such
>> > that it could be safely cast to immutable. That being the 
>> > case,
>> > I'd encourage you to not only make it work with immutable 
>> > char
>> > but to make immutable char the default instead of char.
>> 
>> Is is indeed unique and can be cast to immutable. I'll add that
>> option.
>> Why do you think immutable char as the default is better than
>> char? I know that the return type in that case would be string
>> and not char[] - but why is that better?
>
> strings can't be sliced with impunity, because their elements 
> are immutable. With char[] and const(char)[], you have to worry 
> about the elements changing, so you're often forced to 
> duplicate the string rather than simply slice it. As such, 
> string is far preferrable to char[] or const(char)[] in the 
> general case. It's even better when there's a choice between 
> them, but if you have to choose, you should almost always 
> choose string (there are exceptions though - e.g. buffers like 
> in std.stdio.ByLine where it reuses its char[] buffer rather 
> than allocating a new one).
>
>> > Please make sure that opening braces on on their own line.
>> > That's the way that the rest of Phobos does it and it's one 
>> > of
>> > the few formatting rules that we've been trying to use
>> > consistently throughout Phobos. For the most part, you get it
>> > right, but not everywhere - nested functions in particular 
>> > seem
>> > to have braces on the same line for some reason.
>>
>> When I look through the code it seems ok with regards to braces
>> on own line. Maybe by nested functions you mean delegates? The
>> delegates I use do indeed have braces on same line which is ok
>> afaik.
>
> In general, just make sure that braces are on their own line 
> unless you're dealing with a one line lambda or something 
> similar. You do have nested functions or delegates which don't 
> do that.
>
>> > It's not a big deal, but you should use auto more. For
>> > instance, lines #626, #638, and #640 don't use auto when they
>> > could.
>> 
>> I'm a bit in doubt. On one hand it is great to make everything
>> auto in order to make it easy to change type and to remove
>> redundancy. One the other hand it is very convenient to be able
>> to see the type when you read the code. I know that a clever
>> editor would be able to figure out the type for me. But I also
>> read code in normal editors for diffs etc. or on github.
>> 
>> Anyway - I've changed as much as possible to auto now.
>
> Just in general, it's best practice to use auto unless you 
> can't. There _are_ exceptions to that rule, but that's almost 
> always the way that it should be.
>
>> > I'd suggest changing line# 823 to
>> > 
>> > auto result = get(url, isFtpUrl(url) ? FTP() : Http());
>> 
>> Not possible. get() is a template function where the template
>> parameter is connection type ie. one of FTP or HTTP.
>> isFtpUrl(url) is evaluated at runtime and not compile time.
>
> Yeah. I missed that. Stuff like that makes me wish that we had 
> some kind of static ternary operator, but it would probably 
> complicate the language too much. In some cases though, you 
> might be able to use alias to fix the problem (static if the 
> alias and then use the alias in the function call so that the 
> entire function call isn't duplicated).

A static ternary operator wouldn't work in this case since 
isFtpUrl(url) cannot be evaluated at compile time.

/Jonas