You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2005/04/28 08:02:59 UTC
Re: svn commit: r14478 - in trunk/subversion: tests/libsvn_subr
On Wed, 27 Apr 2005 philip@tigris.org wrote:
> Author: philip
> Date: Wed Apr 27 11:43:53 2005
> New Revision: 14478
>
> Modified:
> trunk/subversion/libsvn_subr/path.c
> trunk/subversion/tests/libsvn_subr/path-test.c
>
> Modified: trunk/subversion/libsvn_subr/path.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_subr/path.c?rev=14478&p1=trunk/subversion/libsvn_subr/path.c&p2=trunk/subversion/libsvn_subr/path.c&r1=14477&r2=14478
> ==============================================================================
> --- trunk/subversion/libsvn_subr/path.c (original)
> +++ trunk/subversion/libsvn_subr/path.c Wed Apr 27 11:43:53 2005
> @@ -712,33 +712,12 @@
> {
> apr_size_t j;
>
> - assert (path);
> + for(j = 0; path[j]; ++j)
Missing space before paren.
> + if (path[j] == ':' || path[j] == '/')
> + break;
>
Please put multiline statements in braces.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14478 - in trunk/subversion: tests/libsvn_subr
Posted by Daniel Rall <dl...@finemaltcoding.com>.
On Thu, 2005-04-28 at 19:00 +0100, Philip Martin wrote:
>"Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
>> On Thu, 28 Apr 2005, Philip Martin wrote:
>>
>>> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>>>
>>> >> + if (path[j] == ':' || path[j] == '/')
>>> >> + break;
>>> >>
>>> > Please put multiline statements in braces.
>>>
>>> I don't think that is an improvement.
>>>
>> It was pointed out to me before that that's our convention.
>
>Really? I wasn't aware we had such a convention. I don't think the
>code in question needs braces, if some convention "requires" that they
>be added then I think the convention should be changed. Stupid rules
>don't help.
>
Perhaps showing my ignorance here, but I understood the "multi-line"
portion of that rule to apply to the block, not the conditional plus the
block. From what I've seen, that matches the existing usage pattern
more closely, and is generally a more useful guideline for improving
code comprehensibility.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14478 - in trunk/subversion: tests/libsvn_subr
Posted by Philip Martin <ph...@codematters.co.uk>.
Greg Hudson <gh...@MIT.EDU> writes:
> I don't think it's consistent with our code base to have multi-line (but
> single-statement) for loop bodies without braces. Every rule of style
> can seem like a "stupid rule" to someone who either isn't used to it or
> temperamentally doesn't benefit from it, but consistency is more
> important than making every developer happy.
I've written code for years to many different "coding standards", some
of which were laughable, and I agree that consistency is important;
consistency is probably more important than which standard is chose.
However think standards should try to avoid "foolish consistency" and
any standard that requires
for(j = 0; path[j]; ++j)
if (path[j] == ':' || path[j] == '/')
break;
to be written as
for(j = 0; path[j]; ++j)
{
if (path[j] == ':' || path[j] == '/')
break;
}
is foolish in my view, in the same way that I would object if required
to write
if ((path[j] == ':') || (path[j] == '/'))
> (And yes, in retrospect I
> think we should have been consistent about space-before-paren for
> function calls across the entire source code base, even if I'm one of
> the dissenters on that front.)
I'd also like us to choose one or the other, I don't care which.
> That said, I don't think there's any specific rule about this in HACKING
> or in the GNU C standards.
The file in question contains other instances of code formatted in the
style I used.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14478 - in trunk/subversion: tests/libsvn_subr
Posted by Greg Hudson <gh...@MIT.EDU>.
(The code at issue follows; Peter asked that the for loop body be put in
braces.)
+ for(j = 0; path[j]; ++j)
+ if (path[j] == ':' || path[j] == '/')
+ break;
On Thu, 2005-04-28 at 14:00, Philip Martin wrote:
> Really? I wasn't aware we had such a convention. I don't think the
> code in question needs braces, if some convention "requires" that they
> be added then I think the convention should be changed. Stupid rules
> don't help.
I don't think it's consistent with our code base to have multi-line (but
single-statement) for loop bodies without braces. Every rule of style
can seem like a "stupid rule" to someone who either isn't used to it or
temperamentally doesn't benefit from it, but consistency is more
important than making every developer happy. (And yes, in retrospect I
think we should have been consistent about space-before-paren for
function calls across the entire source code base, even if I'm one of
the dissenters on that front.)
That said, I don't think there's any specific rule about this in HACKING
or in the GNU C standards.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14478 - in trunk/subversion: tests/libsvn_subr
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> On Thu, 28 Apr 2005, Philip Martin wrote:
>
>> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>>
>> >> + if (path[j] == ':' || path[j] == '/')
>> >> + break;
>> >>
>> > Please put multiline statements in braces.
>>
>> I don't think that is an improvement.
>>
> It was pointed out to me before that that's our convention.
Really? I wasn't aware we had such a convention. I don't think the
code in question needs braces, if some convention "requires" that they
be added then I think the convention should be changed. Stupid rules
don't help.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14478 - in trunk/subversion: tests/libsvn_subr
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 28 Apr 2005, Philip Martin wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
> >> + if (path[j] == ':' || path[j] == '/')
> >> + break;
> >>
> > Please put multiline statements in braces.
>
> I don't think that is an improvement.
>
It was pointed out to me before that that's our convention.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14478 - in trunk/subversion: tests/libsvn_subr
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
>> {
>> apr_size_t j;
>>
>> - assert (path);
>> + for(j = 0; path[j]; ++j)
>
> Missing space before paren.
OK.
>> + if (path[j] == ':' || path[j] == '/')
>> + break;
>>
> Please put multiline statements in braces.
I don't think that is an improvement.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org