[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
Re: commit sizes, coding conventions, and back-ldif
Howard Chu wrote:
[Rearranging a little]
> And it's always preferable to have small, well-defined bug reports in
> the ITS, so we can have one patch = one ITS. In this case, ITS#5408 is
> quite a bundle.
OK, I'll keep that in mind next time. In this case I wanted to avoid
several OpenLDAP versions with incompatible changes and no particular
plan for the transition, but I could have noted that in the ITSes.
I'm getting confused below:
> (...) Since you've chosen to not to commit any intermediate patches
> as you developed them, I don't think it really makes sense to
> artificially recreate those intermediate steps after the fact. (...)
Okay, but...
> If there were well defined functional boundaries for the patches, then
> that would still be worth preserving in separate commits, particularly
> if it were possible to roll back one patch and still keep everything
> else. It's always preferable to have smaller patches.
Er, yes, that's what I thinking of. One commit per logical issue
(listed in the ITS). Each committed version should work - or work
better than the previous version anyway, without new bugs. Can do.
Or did you mean should do next time, since you say:
> If the new code is going to be so different anyway, it may be best to
> only do two commits - first with a pure reformat of the existing code
> (and no syntactic changes), and then with the bugfix.
Syntactic changes? The syntax is C:-) There are various "no-op" changes
- reformatting, moving code between functions, changing function
parameters, return types and a struct, renaming variables. Factoring
common code out of functions, though that's not a no-op since each
function differed slightly (and incorrectly:-)
Now that I think of it I could undo some of that and the patch will be
smaller, since I originally did it when I modified related code and then
later fixed an issue differently. Or I could go all the way and
reformat everything, rename variables with abandon, and split up a
200-line function just for readability.
> At the moment I see no compelling reason to split back-ldif into
> multiple files.
Fine.
>> Regarding formatting, (...)
> Just offering my personal views, nothing official here.
> (...)
I think you are now officially the most active contributor, so it makes
sense to try to follow your style though:-)
>> However often but not always whitespace between two parens are omitted -
>> e.g. "foo( bar( baz ))". Is there some preferred guideline about that?
>>
>> How about whitespace around parens that are for grouping?
>> E.g. "if ( (ch = getc(f)) != EOF )".
>
> I prefer no whitespace between consecutive parens. Naked parens always
> look like a syntax error to me, they're an immediate distraction that
> I then have to dismiss before focusing on the point of interest.
I have the same feeling about most of the OpenLDAP spaces inside parens,
so for me it's a question of which weird and ugly style to pick.
So, "if (( ch = getc( f )) != EOF )"? Or did you only mean,
not "if ( ( ch = getc( f ) ) != EOF )"? (I notice I made a mistake
with my example, "getc(f)" should in any case have been "getc( f )".)
>> * Similarly generous use of {} for if statements etc, usually used even
>> when enclosing just a single statement.
>
> I prefer no brackets for single statements, but I recognize that this
> leads to inconsistencies.
Yup, me too. I tend to find briefer code more readable. For the
same reason usually I prefer
a = x ? y : z;
over
if ( x ) {
a = y;
} else {
a = z;
}
I really don't get the reason for writing the latter unless the expression
gets complex - in addition to more code to read, it even makes it a bit
harder to notice that all (or both) branches modify 'a'.
> (Particularly if the single statement is itself an if, and there is an
> else clause involved. In that case you're forced to use brackets
> anyway to disambiguate...)
Yup.
>> * Indent 1 tab for continuation lines with the same paren level, e.g.
>> x = foo(
>> bar, baz );
>> (...)
>> OpenLDAP code often does put arguments after the '(' but to my eyes it
>> looks easier to read without that, in addition to making emacs happy.
>
> I prefer as many arguments as will fit on the first line. Excessive line
> breaks make it harder to grep for meaningful patterns.
Good point. Oh well. I do think it makes sense to group related
arguments together if that doesn't lead to more lines though, e.g.
foo( a,
buf, sizeof(buf) );
rather than
foo( a, buf,
sizeof(buf) );
>> * Tab-width = 4 columns. Makes a difference for when to break lines
>> (keeping line length< 80), how to align comments after code on a
>> line, and to align declarations and multi-line statements.
>
> Yes, that's pretty much carved in stone.
Which is why I'd like to reformat the entire source tree:-( Good excuse
for changing to a style which Unix Indent and Emacs can handle though.
Though Emacs handles tab-width 4 fine. I have (roughly) this in .emacs:
(defun my-c-mode-hook ()
(modify-syntax-entry ?_ "w") ; Treat '_' like alphanumerics in C
(setq case-fold-search nil) ; C is case-sensitive
(c-set-style "stroustrup") ; Roughly the correct indentation style
(when (string-match "ldap[^/]*/" (or buffer-file-name default-directory ""))
(setq tab-width 4))) ; Tab stops = 4 columns
(add-hook 'c-mode-common-hook 'my-c-mode-hook)
>> It doesn't hurt to try to keep code nice-looking with tab width 8 too.
>> (E.g. comments above statements instead of aligned to the right, and
>> often do not align variables in declarations.)
>
> Not a consideration. 8-column tabs would spill a lot of lines over 80 columns
> but there's no reason to worry about that.
OK.
>> * Usually align comments/decls to the right of code with tabs too, but
>> not always. (If we used space to align after code, it would stay
>> aligned regardless of tab-width.)
>
> I prefer tabs.
OK. Well, in case of the variable names in declarations I prefer a
single space, then it doesn't matter if someone who uses tab-width 8
wrote the declaration.
--
Hallvard