Second Round CURL Wrapper Review
jdrewsen at nospam.com
Sun Dec 18 12:49:16 PST 2011
On Sunday, 18 December 2011 at 01:27:45 UTC, 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
>> > 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
>> function. It just did not fail because libcurl requires the
>> setup for this download situation for both ftp and http. Will
>> 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
>> 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
> 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.
More information about the Digitalmars-d