You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Nick Kew <ni...@webthing.com> on 2006/10/02 20:56:49 UTC
Coding style
We have a bunch of new bug reports[1], detailing bugs of the form
if ((rv = do_something(args) == APR_SUCCESS))
for
if ((rv = do_something(args)) == APR_SUCCESS)
Of course, that's a C classic, and can be a *** to spot.
We can avoid this by adopting an alternative coding style
that doesn't rely on confusing parentheses:
if (rv = do_something(args), rv == APR_SUCCESS)
Can I suggest adopting this as a guideline for new code,
to avoid this kind of bug?
[1] PR#40656 and 40658-40662
--
Nick Kew
Re: Coding style
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Oct 02, 2006 at 05:01:51PM -0700, Roy T. Fielding wrote:
> In any case, I prefer the style of C code that more directly reflects
> the underlying assembly, even if an optimizing compiler would produce
> the same assembler for both. It is just natural to read "use the result
> of this save operation for the test", as opposed to "save the result of
> the operation, discard it, read it back into memory and then test".
I agree with this. A conditional test should scan like a boolean logic
expression. The "stop, discard result" semantics of the comma operator
prevents that.
joe
Re: Coding style
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Oct 2, 2006, at 3:54 PM, Nick Kew wrote:
> On Monday 02 October 2006 23:31, Roy T. Fielding wrote:
>
>>> We can avoid this by adopting an alternative coding style
>>> that doesn't rely on confusing parentheses:
>>>
>>> if (rv = do_something(args), rv == APR_SUCCESS)
>>
>> Which nobody uses because it is so weird and unusual (speaking as
>> a perl
>> programmer who uses even more weird and unusual syntax all the time).
>
> Not a Lisp programmer, I take it? (lisp is full of comma-lists)
> I often use them in C, and have done since nineteeneightysomething.
Lisp is a fine language. Very useful for inducing blindness.
I just don't use it when I want to write readable C code, for the
same reason I don't use perl-style shortcuts (which actually originated
in C, sh, and awk, but are rarely used now in C).
>> It wouldn't surprise me at all to find that it doesn't even
>> compile with
>> some of the older C compilers, though I haven't tested it.
>
> It's been clearly defined by ANSI C for as long as ANSI C has existed.
> I'm pretty sure it was K&R before that, too.
Yes, it was, but is not commonly used in practice aside from within
declaration lists and for-loop increments. Personally, I think it is
because a comma is too difficult to see on a screen, but it may also
be because a parenthesized list of expressions looks just like a list
of function call parameters. *shrug*
In any case, I prefer the style of C code that more directly reflects
the underlying assembly, even if an optimizing compiler would produce
the same assembler for both. It is just natural to read "use the result
of this save operation for the test", as opposed to "save the result of
the operation, discard it, read it back into memory and then test".
YMMV. I don't think that commas are any less error-prone than parens.
....Roy
Re: Coding style
Posted by Nick Kew <ni...@webthing.com>.
On Monday 02 October 2006 23:31, Roy T. Fielding wrote:
> > We can avoid this by adopting an alternative coding style
> > that doesn't rely on confusing parentheses:
> >
> > if (rv = do_something(args), rv == APR_SUCCESS)
>
> Which nobody uses because it is so weird and unusual (speaking as a perl
> programmer who uses even more weird and unusual syntax all the time).
Not a Lisp programmer, I take it? (lisp is full of comma-lists)
I often use them in C, and have done since nineteeneightysomething.
> It wouldn't surprise me at all to find that it doesn't even compile with
> some of the older C compilers, though I haven't tested it.
It's been clearly defined by ANSI C for as long as ANSI C has existed.
I'm pretty sure it was K&R before that, too.
--
Nick Kew
Re: Coding style
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Oct 2, 2006, at 11:56 AM, Nick Kew wrote:
> We have a bunch of new bug reports[1], detailing bugs of the form
>
> if ((rv = do_something(args) == APR_SUCCESS))
>
> for
>
> if ((rv = do_something(args)) == APR_SUCCESS)
>
> Of course, that's a C classic, and can be a *** to spot.
Huh, just turn on the warnings. Or use lint.
> We can avoid this by adopting an alternative coding style
> that doesn't rely on confusing parentheses:
>
> if (rv = do_something(args), rv == APR_SUCCESS)
Which nobody uses because it is so weird and unusual (speaking as a perl
programmer who uses even more weird and unusual syntax all the time).
It wouldn't surprise me at all to find that it doesn't even compile with
some of the older C compilers, though I haven't tested it.
I prefer the existing convention. +1 on fixing the bugs.
....Roy
Re: Coding style
Posted by Brian Akins <br...@turner.com>.
Garrett Rooney wrote:
> Or the even more readable:
>
> rv = do_something(args);
> if (rv == APR_SUCCESS) {
>
> }
yuck! Think of all the harmless newlines you are senselessly wasting.
Our children will have to code with no newlines if we do not conserve
them today. Won't someone please think of the children.
Seriously, not a fan of that style. Doesn't matter that much to me,
however.
-0
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: Coding style
Posted by Niklas Edmundsson <ni...@acc.umu.se>.
On Mon, 2 Oct 2006, Garrett Rooney wrote:
> Or the even more readable:
>
> rv = do_something(args);
> if (rv == APR_SUCCESS) {
>
> }
+1 for simple code like this.
It comes naturally when you need to do stuff like
rv = dostuff(...);
if(rv != APR_SUCCESS && rv != whatever) {
...
and is also less likely to cause ugly linewraps when using
functions_with_long_names(and, a, large, list, of, arguments) ...
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | nikke@acc.umu.se
---------------------------------------------------------------------------
Sexy: Uses feather. Kinky: Uses entire chicken.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Re: Coding style
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 10/2/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
>
> Or the even more readable:
>
> rv = do_something(args);
> if (rv == APR_SUCCESS) {
>
> }
+1.
Using the comma operator is orders of magnitude too nasty/subtle to use. --
justin
Re: Coding style
Posted by Bill Stoddard <bi...@wstoddard.com>.
Jeff Trawick wrote:
>> Or the even more readable:
>>
>> rv = do_something(args);
>> if (rv == APR_SUCCESS) {
>>
>> }
>
> +1 to this one
>
That's my preference as well. Unambiguous and easy to read.
Bill
Re: Coding style
Posted by Jeff Trawick <tr...@gmail.com>.
On 10/2/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 10/2/06, Nick Kew <ni...@webthing.com> wrote:
> > We have a bunch of new bug reports[1], detailing bugs of the form
> >
> > if ((rv = do_something(args) == APR_SUCCESS))
> >
> > for
> >
> > if ((rv = do_something(args)) == APR_SUCCESS)
> >
> > Of course, that's a C classic, and can be a *** to spot.
> >
> > We can avoid this by adopting an alternative coding style
> > that doesn't rely on confusing parentheses:
> >
> > if (rv = do_something(args), rv == APR_SUCCESS)
> >
> > Can I suggest adopting this as a guideline for new code,
> > to avoid this kind of bug?
>
> Or the even more readable:
>
> rv = do_something(args);
> if (rv == APR_SUCCESS) {
>
> }
+1 to this one
Re: Coding style
Posted by Maxime Petazzoni <ma...@bulix.org>.
Hi,
* Ruediger Pluem <rp...@apache.org> [2006-10-02 23:35:03]:
> Ok. Point taken. So I guess the best thing for simple if (...) is
> Garrett's proposal (put the function call *before* if in a separate
> line). For more complex things like the thing above (provided that
> we want to allow this at all) the comma-list is better.
I've been silently following this coding style discussion with a bit
of fun I must say, but I'd like to +1 this sum up.
- Sam
--
Maxime Petazzoni (http://www.bulix.org)
-- gone crazy, back soon. leave message.
Re: Coding style
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/02/2006 11:12 PM, Nick Kew wrote:
> On Monday 02 October 2006 21:39, Ruediger Pluem wrote:
>
>
>>In general I agree with the above, but there are situations were the old
>>style really makes sense, e.g (from log_child in server/log.c):
>>
>>if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
>> && ((rc = apr_procattr_cmdtype_set(procattr,
>> APR_SHELLCMD_ENV)) ==
>>APR_SUCCESS) && ((rc = apr_procattr_io_set(procattr,
>> APR_FULL_BLOCK,
>> APR_NO_PIPE,
>> APR_NO_PIPE)) == APR_SUCCESS)
>> && ((rc = apr_procattr_error_check_set(procattr, 1)) ==
>>APR_SUCCESS) && ((rc = apr_procattr_child_errfn_set(procattr,
>>log_child_errfn)) == APR_SUCCESS)) {
>
>
> Now that is true bug-heaven. If we accept your premise that a complex
> if (...) is better than the alternatives, then a comma-list is much less
> bug-prone:
>
> if ((rc = apr_procattr_create(&procattr, p), rc == APR_SUCCESS)
> && (rc = apr_procattr_cmdtype_set(procattr, APR_SHELLCMD_ENV),
> rc == APR_SUCCESS)
> && (rc = apr_procattr_io_set(procattr, APR_FULL_BLOCK,
> APR_NO_PIPE, APR_NO_PIPE),
> rc == APR_SUCCESS)
> && (rc = apr_procattr_error_check_set(procattr, 1), rc == APR_SUCCESS)
> && (rc = apr_procattr_child_errfn_set(procattr, log_child_errfn),
> rc == APR_SUCCESS)) {
>
> (Not sure how that'll look in terms of wrap and whitespace -
> this is a mailer, not a program editor. But you get the point).
Ok. Point taken. So I guess the best thing for simple if (...) is Garrett's
proposal (put the function call *before* if in a separate line). For more
complex things like the thing above (provided that we want to allow this at all)
the comma-list is better.
Regards
Rüdiger
Re: Coding style
Posted by Nick Kew <ni...@webthing.com>.
On Monday 02 October 2006 21:39, Ruediger Pluem wrote:
> In general I agree with the above, but there are situations were the old
> style really makes sense, e.g (from log_child in server/log.c):
>
> if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
> && ((rc = apr_procattr_cmdtype_set(procattr,
> APR_SHELLCMD_ENV)) ==
> APR_SUCCESS) && ((rc = apr_procattr_io_set(procattr,
> APR_FULL_BLOCK,
> APR_NO_PIPE,
> APR_NO_PIPE)) == APR_SUCCESS)
> && ((rc = apr_procattr_error_check_set(procattr, 1)) ==
> APR_SUCCESS) && ((rc = apr_procattr_child_errfn_set(procattr,
> log_child_errfn)) == APR_SUCCESS)) {
Now that is true bug-heaven. If we accept your premise that a complex
if (...) is better than the alternatives, then a comma-list is much less
bug-prone:
if ((rc = apr_procattr_create(&procattr, p), rc == APR_SUCCESS)
&& (rc = apr_procattr_cmdtype_set(procattr, APR_SHELLCMD_ENV),
rc == APR_SUCCESS)
&& (rc = apr_procattr_io_set(procattr, APR_FULL_BLOCK,
APR_NO_PIPE, APR_NO_PIPE),
rc == APR_SUCCESS)
&& (rc = apr_procattr_error_check_set(procattr, 1), rc == APR_SUCCESS)
&& (rc = apr_procattr_child_errfn_set(procattr, log_child_errfn),
rc == APR_SUCCESS)) {
(Not sure how that'll look in terms of wrap and whitespace -
this is a mailer, not a program editor. But you get the point).
--
Nick Kew
Re: Coding style
Posted by Joachim Zobel <jz...@heute-morgen.de>.
Am Montag, den 02.10.2006, 22:39 +0200 schrieb Ruediger Pluem:
> if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
> && ((rc = apr_procattr_cmdtype_set(procattr,
> APR_SHELLCMD_ENV)) == APR_SUCCESS)
> && ((rc = apr_procattr_io_set(procattr,
> APR_FULL_BLOCK,
> APR_NO_PIPE,
> APR_NO_PIPE)) == APR_SUCCESS)
> && ((rc = apr_procattr_error_check_set(procattr, 1)) == APR_SUCCESS)
> && ((rc = apr_procattr_child_errfn_set(procattr, log_child_errfn)) == APR_SUCCESS)) {
>
> To get the same thing with above style you would create a lot
> of nested if statements or you would have to use a lot of if statements and goto.
I would put this in a seperate function where it turns into the easy to
read classic, that returns after any unsuccessfull call. These returns
are hidden gotos, of course.
Sincerely,
Joachim
Re: Coding style
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/02/2006 08:59 PM, Garrett Rooney wrote:
> On 10/2/06, Nick Kew <ni...@webthing.com> wrote:
>
>> We have a bunch of new bug reports[1], detailing bugs of the form
>>
>> if ((rv = do_something(args) == APR_SUCCESS))
>>
>> for
>>
>> if ((rv = do_something(args)) == APR_SUCCESS)
>>
>> Of course, that's a C classic, and can be a *** to spot.
>>
>> We can avoid this by adopting an alternative coding style
>> that doesn't rely on confusing parentheses:
>>
>> if (rv = do_something(args), rv == APR_SUCCESS)
>>
>> Can I suggest adopting this as a guideline for new code,
>> to avoid this kind of bug?
>
>
> Or the even more readable:
>
> rv = do_something(args);
> if (rv == APR_SUCCESS) {
>
> }
In general I agree with the above, but there are situations were the old
style really makes sense, e.g (from log_child in server/log.c):
if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
&& ((rc = apr_procattr_cmdtype_set(procattr,
APR_SHELLCMD_ENV)) == APR_SUCCESS)
&& ((rc = apr_procattr_io_set(procattr,
APR_FULL_BLOCK,
APR_NO_PIPE,
APR_NO_PIPE)) == APR_SUCCESS)
&& ((rc = apr_procattr_error_check_set(procattr, 1)) == APR_SUCCESS)
&& ((rc = apr_procattr_child_errfn_set(procattr, log_child_errfn)) == APR_SUCCESS)) {
To get the same thing with above style you would create a lot
of nested if statements or you would have to use a lot of if statements and goto.
Regards
Rüdiger
Re: Coding style
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/2/06, Nick Kew <ni...@webthing.com> wrote:
> We have a bunch of new bug reports[1], detailing bugs of the form
>
> if ((rv = do_something(args) == APR_SUCCESS))
>
> for
>
> if ((rv = do_something(args)) == APR_SUCCESS)
>
> Of course, that's a C classic, and can be a *** to spot.
>
> We can avoid this by adopting an alternative coding style
> that doesn't rely on confusing parentheses:
>
> if (rv = do_something(args), rv == APR_SUCCESS)
>
> Can I suggest adopting this as a guideline for new code,
> to avoid this kind of bug?
Or the even more readable:
rv = do_something(args);
if (rv == APR_SUCCESS) {
}
-garrett
Re: Coding style
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/02/2006 11:15 PM, Joachim Zobel wrote:
> Am Montag, den 02.10.2006, 19:56 +0100 schrieb Nick Kew:
>
>>Can I suggest adopting this as a guideline for new code,
>>to avoid this kind of bug?
>
>
> Is there an apache coding style guide?
Yes of course:
http://httpd.apache.org/dev/styleguide.html
Regards
Rüdiger
Re: Coding style
Posted by Joachim Zobel <jz...@heute-morgen.de>.
Am Montag, den 02.10.2006, 19:56 +0100 schrieb Nick Kew:
> Can I suggest adopting this as a guideline for new code,
> to avoid this kind of bug?
Is there an apache coding style guide?
Thx,
Joachim