On Mon, Jun 28, 2010 at 08:55:57PM -0400, Wesley Craig wrote:
> On 28 Jun 2010, at 18:21, Bron Gondwana wrote:
> >Parantheses cuddle close
>
> This is not how I write C (as you know...), tho I recognize that the
> majority of Cyrus is written this way.
Yeah, I know. That's what I'm having a whinge about right now.
Cyrus style isn't my preferred way of writing code either - but
when you're working in a code base, you have to respect the
style that code base is written in or it gets all fragmented and
harder for everyone to work on. So let's make a compromise we
can all sort-of live with, and hold our noses while we're doing
Cyrus dev!
> I also "over space"
> statements like:
>
> HASH || DASH
Agreed. Spacing around double bars is correct IMHO.
> because I've been burned trying to find cases where I meant the
> above but had written:
>
> HASH|DASH
And no sparcing around single bars. I'm happy with that.
if (a || b) {
foo = FLAG_A|FLAG_B;
}
Similarly, space around mathematical operators. if (a > b) to
clarify that it's not a typo of if (a->b).
> >* braces are optional if meaning is clear
> > if (foo)
> > function();
> > else
> > other_function();
>
> Personally, I always include the brace, with the exception of one-
> liners, e.g.:
>
> if ( foo ) goto blah;
I think that's a better approach too, but there's HEAPS of code in
Cyrus at the moment that's not that way. If we decide to say
"unless it's on the same line it has braces" then I'm happy to
say that and start making all new code follow it.
How do you feel about:
if (a) function_one(a);
else function_two();
> and other one-liners. I've debugged many instances of (to use your
> example):
>
> if (foo)
> function();
> else
> other_function();
> added_by_someone_who_did_not_read_carefully();
Yes, we all have! Speaking of which, snprintf(foo, sizeof(foo), ...)
is a bogus pattern. It works right until you refactor the block of
code and it's char *foo, not char foo[MAX_LEN]. It's good to make
anti-patterns hard.
> >* goto is permitted within a function only
>
> As opposed to long jump? My rule for thumb for goto's is:
>
> 1) to the end of a function for cleanup
> 2) occasionally, to the top of a loop, but only if using another
> control structure is *less* clear
That's how it's used within Cyrus now. I would like to keep that
"allowed" by the coding standard.
> >Anything else anyone want to add?
>
> Lots, I'm sure, but nothing that would cause me to review the body
> of Cyrus code and re-style it.
It's OK not to restyle straight away so long as there's a consensus
that we want to head that way. I'm pretty strongly opposed to your
wide-spaced outer brackets, I don't think they gain anything, and
they're not a common C idiom.
> >* use "const char *" where possible.
>
> While this is a fine idea, I don't think it's well practiced in the
> Cyrus code base. Moving in this direction, over time, as code is
> touched, is a nice idea.
I've added heaps of it actually as part of the -future branch, because
I wanted to make it const in some structs, and the rest flowed from
that. Also, some time with -Wall and static code analysis found a
bunch of stuff I've tidied up.
Along with bit32 usage, this would tidy up "types" a lot.
> >* RAII where possible.
>
> Since C has no such concept, I assume you mean "goto (maybe several)
> the end(s) of a function and clean up"? Or are you thinking of
> constructor/destructor class stuff? Neither of these is well
> practices in the Cyrus code base.
Yeah - the "multiple goto targets at the end" is one way to do it.
The other way is to have a wrapper function that does the setup and
cleanup, and calls a worker function that does the interesting bits
and can just return an error code.
Bron.
