Second Round CURL Wrapper Review

[Jonathan M Davis]

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

- Jonathan M Davis