On Mon, 2009-06-22 at 21:24 -0400, Duane Ellis wrote:
> Zach Welch wrote:
> > There have been no objections to these series of patches, so I intend to
> > regenerate and apply them soon.
> >
> There is one thing I do not like - not exactly what you are talking
> about here.. I'd rather my voice be heard...
>
> In general, I think a general "white space cleanup" is nice... and a
> good thing... "Janitor work" - glad you are doing it...
>
> that said, please - I don't want to see us head down the road where
> issues like the ones I out line below come up.
>
> ==========
>
> The general statement is this:
>
> What I do *NOT* like and I find it terribly hard to read - is
> utterly *DENSE* code - that is almost *devoid* of white space in both
> the x & y dimensions. Often, white space is there for a reason - and
> "cleanup tools" - are a tad bit too helpful.
I believe that I have been using whitespace appropriately; while I may
have made the code denser in terms its total number of lines, I think
that my changes have been slowly improving the "readability density".
> I've been burned by them before!
>
> ==========
> Examples include
> ==========
>
> (a) multiple 'format specifiers' to a printf() like function - counting
> function parameters to find 'argument 12' - is daunting.
>
> A specific example: *this* 258 char long printf() statement.
>
> printed = snprintf(buf, buf_size, "protection_fields: %i, prot_reg_addr:
> 0x%x, factory pre-programmed: %i, user programmable: %i\n",
> pri_ext->num_protection_fields, pri_ext->prot_reg_addr, 1 <<
> pri_ext->fact_prot_reg_size, 1 << pri_ext->user_prot_reg_size);
>
> Imagine that - 258 bytes - and that one does not need the <c99> format
> specifiers! Wow!
>
> At some point, a *single*column* of parameters, one per line... is much
> better!
> Might not fit some style guide... but please - line length > 150 is sort
> of absurd.
I updated the Style Guide to recommend < 80 columns. Almost any code
will be more readable if factored to fit into such dimensions, so you
are preaching to the choir.
> What I talk about adds white space all over the function call - but
> improves readablity.
All of the patches in these series add whitespace, except for the last
that removes it at end of lines, after '(', and before ')'.
> ============
> Collapsed IF statements
> ==========
>
> (b) - Collapsing if() statements that - just - in the end make the
> statement *longer* on the line..
>
> Example (bad, 127 char line of code, example from "cfi.c")
>
> if((retval = target_write_memory(target, flash_address(bank, 0,
> pri_ext->_unlock2), bank->bus_width, 1, command)) != ERROR_OK)
> {
> return retval;
> }
>
> Example (rewrite-good, use a temp variable, to kill line length)
>
> { uint32_t adr;
> adr = flash_address(bank, 0, pri_ext->_unlock2)
> retval = target_write_memory(target, adr, bank->bus_width, 1,
> command);
> }
> if( retval != ERROR_OK)
> {
> return retval;
> }
I am with you here except for all of the curly braces; those are nearly
superfluous. I think they make the code more "dense" than it should be.
Otherwise, you are right in saying that such lines should be broken out
into separate "do it" and "check it" statements.
> ==============
> White space removal in initialized data, or function calls.
> ==============
>
> (c) Often, a *LONG* table of data is created, and great pain is taken to
> *ALIGN* commas, parens, and other elements of the data such that things
> line up in a nice neat tabular format,while the whitespace does not meet
> "style guide rules" it is there for other reasons - improved readability
> and comprehension..
I skipped commas for this reason. I saw that it would disrupt more than
it helps.
> yea, truthfully - compliance with this is all over the map... it does
> vary quite a bit.
Yup, but these patches will bring almost all operator whitespace issues
into compliance; only a handful of operators will remain to fix later.
Cheers,
Zach
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
