You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2010/03/11 19:42:57 UTC
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
On Thu, Mar 11, 2010 at 08:52, <ne...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Thu Mar 11 13:52:15 2010
>...
> @@ -2219,9 +2229,32 @@ svn_wc__get_pristine_contents(svn_stream
> return SVN_NO_ERROR;
> }
> }
> + else
> + if (status == svn_wc__db_status_base_deleted)
Woah. This formatting is incorrect. We always do "else if (...". The
code above almost makes it look like the "if" is totally separate
since it is at the same indentation level, but it ISN'T. The else
dramatically changes the meaning.
The above style is used nowhere else in our code. Please fix the
several uses in this function.
>...
Cheers,
-g
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by Neels J Hofmeyr <ne...@elego.de>.
Hyrum K. Wright wrote:
>> Hm, I've used this before, always have.
>> IMHO it looks much better this way, and is easier to edit around...
>> I do take care to have the 'else' in the line just above 'if'.
>
> It's unfortunate if this style has found its way into other places in our codebase.
>
>> But whatever, if Greg is surprised by it, not many people will be using my
>> way. Will fix, but now it's high time for bed.
>
> I don't think it's a question of anybody being surprised, or one style is better than another or Greg being the bees-knees or anything like that. Long ago we picked a coding standard, and it just happened to be the "else if (..." style. I'm sure I don't have to enumerate the benefits to being consistent in our style, but suffice it to say it's the way The World is.
>
> (FWIW, I personally prefer "if (...) {", but we don't do that, either. Old habits die hard.)
>
> -Hyrum
I said *whatever* already :)
we have boundaries in which style is allowed. wasn't aware that this is such
a catastrophe... will fix and heed. whatever.
This is definitely less confusing and less important than some of our API
docco. :P
~Neels
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 11, 2010, at 9:11 PM, Neels J Hofmeyr wrote:
> Greg Stein wrote:
>> On Thu, Mar 11, 2010 at 08:52, <ne...@apache.org> wrote:
>>> ...
>>> +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Thu Mar 11 13:52:15 2010
>>> ...
>>> @@ -2219,9 +2229,32 @@ svn_wc__get_pristine_contents(svn_stream
>>> return SVN_NO_ERROR;
>>> }
>>> }
>>> + else
>>> + if (status == svn_wc__db_status_base_deleted)
>>
>> Woah. This formatting is incorrect. We always do "else if (...". The
>> code above almost makes it look like the "if" is totally separate
>> since it is at the same indentation level, but it ISN'T. The else
>> dramatically changes the meaning.
>>
>> The above style is used nowhere else in our code. Please fix the
>> several uses in this function.
>
> Hm, I've used this before, always have.
> IMHO it looks much better this way, and is easier to edit around...
> I do take care to have the 'else' in the line just above 'if'.
It's unfortunate if this style has found its way into other places in our codebase.
> But whatever, if Greg is surprised by it, not many people will be using my
> way. Will fix, but now it's high time for bed.
I don't think it's a question of anybody being surprised, or one style is better than another or Greg being the bees-knees or anything like that. Long ago we picked a coding standard, and it just happened to be the "else if (..." style. I'm sure I don't have to enumerate the benefits to being consistent in our style, but suffice it to say it's the way The World is.
(FWIW, I personally prefer "if (...) {", but we don't do that, either. Old habits die hard.)
-Hyrum
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Mar 12, 2010 at 08:15, neels <ne...@gmail.com> wrote:
> On 12 March 2010 04:11, Neels J Hofmeyr <ne...@elego.de> wrote:
>> Greg Stein wrote:
>>>> + else
>>>> + if (status == svn_wc__db_status_base_deleted)
>>>
>>> Woah. This formatting is incorrect. We always do "else if (...". The
>
> So I've grep'd around for else(\n)if occurences.
> Notably I'm the only one that lined up the 'if' with the 'else'. All
> other occurences indent the 'if'.
>
> hyrum, gstein, would you be fine with me adopting the indented-if
> variant? It pleases my eyes *slightly* more. Feel free to say "nah,
> come on, just use 'else if' and done".
You found 10 occurrences of else\n if.
I found 1239 occurrences of else if.
Guess my answer :-)
Cheers,
-g
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by neels <ne...@gmail.com>.
On 12 March 2010 04:11, Neels J Hofmeyr <ne...@elego.de> wrote:
> Greg Stein wrote:
>>> + else
>>> + if (status == svn_wc__db_status_base_deleted)
>>
>> Woah. This formatting is incorrect. We always do "else if (...". The
So I've grep'd around for else(\n)if occurences.
Notably I'm the only one that lined up the 'if' with the 'else'. All
other occurences indent the 'if'.
hyrum, gstein, would you be fine with me adopting the indented-if
variant? It pleases my eyes *slightly* more. Feel free to say "nah,
come on, just use 'else if' and done".
lundblad(r24045)
subversion/libsvn_client/merge.c:1425: else
subversion/libsvn_client/merge.c-1426- if (prop_state)
--
hwright(r26317) / sussman(r3908)
subversion/svn/propset-cmd.c:91: else
subversion/svn/propset-cmd.c-92- if (opt_state->encoding)
--
kfogel(r5500) / julianfoad (r31978)
subversion/svnlook/main.c:1755: else
subversion/svnlook/main.c-1756- if (xml)
--
neelsr(921848)
subversion/libsvn_wc/adm_ops.c:2231: else
subversion/libsvn_wc/adm_ops.c-2232- if (status ==
svn_wc__db_status_not_present)
--
neels(r906110)
subversion/libsvn_wc/update_editor.c:1775: else
subversion/libsvn_wc/update_editor.c-1776- if (reason ==
svn_wc_conflict_reason_added)
subversion/libsvn_wc/update_editor.c:1861: else
subversion/libsvn_wc/update_editor.c-1862- if (base_kind ==
svn_wc__db_kind_dir)
--
gstein(r35917) / rhuijben(r39084)
subversion/libsvn_wc/old-and-busted.c:197: else
subversion/libsvn_wc/old-and-busted.c-198- if (!
svn_uri_is_canonical(*result, pool))
--
kfogel(r31614)
subversion/tests/libsvn_repos/repos-test.c:2155: else
subversion/tests/libsvn_repos/repos-test.c-2156- if
(strcmp(prop_key, SVN_PROP_REVISION_LOG) != 0)
subversion/tests/libsvn_repos/repos-test.c:2216: else
subversion/tests/libsvn_repos/repos-test.c-2217- if (err->apr_err
!= SVN_ERR_BAD_PROPERTY_VALUE)
subversion/tests/libsvn_repos/repos-test.c:2236: else
subversion/tests/libsvn_repos/repos-test.c-2237- if (err->apr_err
!= SVN_ERR_BAD_PROPERTY_VALUE)
~Neels
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by Neels J Hofmeyr <ne...@elego.de>.
Greg Stein wrote:
> On Thu, Mar 11, 2010 at 08:52, <ne...@apache.org> wrote:
>> ...
>> +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Thu Mar 11 13:52:15 2010
>> ...
>> @@ -2219,9 +2229,32 @@ svn_wc__get_pristine_contents(svn_stream
>> return SVN_NO_ERROR;
>> }
>> }
>> + else
>> + if (status == svn_wc__db_status_base_deleted)
>
> Woah. This formatting is incorrect. We always do "else if (...". The
> code above almost makes it look like the "if" is totally separate
> since it is at the same indentation level, but it ISN'T. The else
> dramatically changes the meaning.
>
> The above style is used nowhere else in our code. Please fix the
> several uses in this function.
Hm, I've used this before, always have.
IMHO it looks much better this way, and is easier to edit around...
I do take care to have the 'else' in the line just above 'if'.
But whatever, if Greg is surprised by it, not many people will be using my
way. Will fix, but now it's high time for bed.
~Neels