[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.


>> 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;
	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...)


>> * 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.


>> * 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.