[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] fix for interactive merge-callback not supporting spaces in file paths

From: C. Michael Pilato <cmpilato_at_collab.net>
Date: 2007-07-23 15:52:28 CEST

Ben Collins-Sussman wrote:
> + if (apr_err)
> + {
> + return svn_error_wrap_apr
> + (apr_err, _("Can't change working directory to '%s'"), base_dir);
> + }
>
> No need for curly braces around a single-line block.
>

[...]

> if (sys_err != 0)
> - /* Extracting any meaning from sys_err is platform specific, so just
> - use the raw value. */
> - return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> - _("system('%s') returned %d"), cmd, sys_err);
> + {
> + /* Extracting any meaning from sys_err is platform specific, so just
> + use the raw value. */
> + err = svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("system('%s') returned %d"), cmd,
> sys_err);
> + }
>
>
> Same thing here.

Actually, Ben, this has always been something we've left up to individual
preferences -- we have no stylistic convention on this matter, and never
have. For one thing, given a block of code like:

   if (some_cond)
     /* This is a comment */
     return (some_stuff);

some would think of that as a one-liner inside the if(), others think of
"lines" like physical lines of text. Arguably, it'd be easier to read this
"one-liner" if it was wrapped in braces.

  if (some_cond)
    /* It is for us the living rather to be dedicated here to the
       unfinished work which they who fought here have thus far so nobly
       advanced. It is rather for us to be here dedicated to the great
       task remaining before us--that from these honored dead we take
       increased devotion to that cause for which they gave the last full
       measure of devotion--that we here highly resolve that these dead
       shall not have died in vain, that this nation under God shall have
       a new birth of freedom, and that government of the people, by the
       people, for the people shall not perish from the earth. */
    return (some_thing);

Other thoughts (YMMV): including the braces up front means that if the body
of the condition grows to more than one line, the diff need only contain the
new logic, not the new formatting of the old logic, too (because indentation
levels change). And there are other consideration, too, like how to format
else blocks -- do you pair an unbraced if() with a braced else, or keep them
visually balanced. Stuff like that. (See even the example in hacking.html
immediately inside the "Coding style" section, as well as inconsistent
examples inside "Exception handling".)

All this to say, try to avoid passing off your personal preference as
Subversion-wide convention. Though, in this case, I suspect you just really
thought such a convention existed.

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Received on Mon Jul 23 15:51:24 2007

This is an archived mail posted to the Subversion Dev mailing list.