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.