etc.curl: Formal review begin

[Jonathan M Davis]

On Monday, August 22, 2011 15:23:54 simendsjo wrote:
> On 22.08.2011 08:25, Jonathan M Davis wrote:
> > On Monday, August 22, 2011 08:09:54 Jonas Drewsen wrote:
> >> On 21/08/11 23.44, Jesse Phillips wrote:
> >>> On Sat, 20 Aug 2011 23:57:54 +0000, Jesse Phillips wrote:
> >>>> On Thu, 18 Aug 2011 01:12:20 +0200, David Nadlinger wrote:
> >>>>> At this point, I would like to invite everyone to spend some
> >>>>> time
> >>>>> testing the module and reading its documentation and code. It is
> >>>>> essential for a working open source code review process that
> >>>>> many
> >>>>> people participate in it, and regardless of whether you are new
> >>>>> to D
> >>>>> or
> >>>>> a seasoned contributor, your opinion is very valuable.
> >>>> 
> >>>> Just tried building a 64 bit Phobos and got:
> >>>> 
> >>>> etc/curl.d(500): Error: function etc.c.curl.curl_easy_escape
> >>>> (void*
> >>>> handle, char* string, int length) is not callable using argument
> >>>> types
> >>>> (void*,char*,ulong)
> >>>> 
> >>>> etc/curl.d(510): Error: function etc.c.curl.curl_easy_unescape
> >>>> (void*
> >>>> handle, char* string, int length, int* outlength) is not callable
> >>>> using
> >>>> argument types (void*,char*,ulong,int*)
> >>>> 
> >>>> A couple casts let it build, moving on to using.
> >>> 
> >>> And now for my miniature review.
> >>> 
> >>> The coding standard for Phobos was decided to have brackets on their
> >>> own line.
> >>> 
> >>> I think the examples should also be limited to about 50-60
> >>> characters in width.
> >>> 
> >>> I don't have much else on comments, once it complied my basic
> >>> testing
> >>> worked. I'd need to have a real objective to actually learn most of
> >>> what it provides, but it seems simple enough. Good work.
> >> 
> >> Thank you for the feedback.
> >> 
> >> There is no mentioning of brackets in the style doc though:
> >> 
> >> http://www.d-programming-language.org/dstyle.html
> > 
> > That's only because it hasn't been updated yet. The 2 main formatting
> > guidelines that aren't in the style guide are
> > 
> > 1. Braces must be on their own line (save perhaps for lambdas which are
> > one liners).
> 
> So I've heard, but even new code doesn't follow this. Davids
> RegionAllocater brand new (in the review queue?), and have braces on the
> same line.

Just because something is in the review queue doesn't mean that it follows the 
style guide correctly. And in a few cases, code which doesn't follow the style 
guide may have managed to get into Phobos because it wasn't noticed or it wsa 
before we decided on what style to use. I believe that David normally puts 
braces on the same line in his own code, so unless he's writing specifically 
for Phobos and remembers to put the braces on their own line, he's likely to 
put them on the same line. It's going to need to be fixed before getting merged 
into Phobos.

- Jonathan M Davis