etc.curl: Formal review begin

[jdrewsen]

Den 22-08-2011 17:20, Jonathan M Davis skrev:
> 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
I've now fixed the 80-120 char limit. Not pushed yet.