Re: [Openocd-development] Nit to pick with recent set of cleanups

[Rick Altherr]

On Jun 15, 2009, at 6:58 PM, Zach Welch wrote:

On Mon, 2009-06-15 at 17:01 -0700, Rick Altherr wrote:

On Jun 15, 2009, at 4:19 PM, Zach Welch wrote:

On Sat, 2009-06-13 at 21:14 -0400, Duane Ellis wrote:

  bool okay = *str && !*end && ULLONG_MAX != *ul;

In my long career, I have seen too many poor souls - including my
self
become the victim of even my own seemingly simple attempts to
reduce the
levels of  () and {}.   Yes, there are cases where it gets a little
too
deep, but there must be a balance.

On the opposite side of the coin, I have seen too many programmers
rely

on extra parentheses because they do not _know_ C operator precedence. There are cases that deserve extra parenthesis, but not on that line.

The extra parentheses and braces do not hurt anything, however.  They
_do_ make it easier for those who are newer to programming or to
clarify the sets of tests being done.  I'm personally not fond of

those using logic operators) tests into a single statement.  If they
are going to be, then I prefer the unnecessarily parentheses be used

incorporating multiple logical (as in separate tests, not specifically to separate the logical tests to make the statement more understandable.

Okay, wait..... you say below that you have written this same code, so
anyone familiar with strtoul (or bothers to read its man page) will

those "newer to programming".  This is not a beginner's project, nor
should developers working on it expect to produce code at that level.

understand exactly what it does. OpenOCD's style should _not_ cater to

No, we should cater to clarity and readability. Code should only be difficult to read and/or understand under a few specific conditions: - Working around an optimizer bug that would otherwise allow the code to be written more simply - Well-commented algrithmic optimizations that are the result of measurement and analysis Since this case is neither, it should be written so that any developer who chooses to look at the code can easily understand it. To do otherwise is generally a sign of elitism or laziness.

The function under scrutiny is _trivial_, but you failed to account for (or notice, perhaps) the fact that I was asking a much broader question in my reply. This one line of code aside: are my cleanups resulting in

a clearer to understand tree?

Yes, I think you have been in general improving the code base. Even in this case you've managed to improve the overall code base even if a few of us have a few nits to pick with it.

Openocd is not, and should never become an entry in the 'obfuscated C

code contest[1]'.  It seems that we are heading down that road.
</screaming-rant>

have to wonder whether others agree with this particular assessment.

Seriously? You think that my efforts have increased the obfuscation? My principle aim has been to improve the readability of the code, so I

I personally find the excerpted line confusing and clunky.  I _do_
know what it does, but only because I've written the exact same code

of the code, here's what I don't like about it:
 - okay is a poor choice of variable name.  Its name declares that it
is "okay", but it really is telling you _if_ something is okay.
is_okay is much clearer.
- There are really two separate tests being done sequentially.  The
first is to see if the whole string was used to generate the number.
The second is to see if the parsed number was larger than an unsigned

associativity means it evaluates that way.
- The use of 'okay' seems potentially unnecessary.  Even if the test
is being used to determine multiple control flow paths, the code can

doesn't really add anything to the clarity.
- If you are testing that a variable is non-NULL, write that.
Similarly for a test for NULL.  Taking the short-cut just makes it
less clear what you are doing and why.  Besides, it isn't necessarily
guaranteed that NULL will always be 0. ;)

to handle the error cases returned by stroul. Without seeing the rest log. That isn't immediately clear even though operator precedence and usually be changed to only need a single if. The variable declaration

This is a four line function!  The okay variable turns a four line if
into two lines of code that I think are very easy to read.  Sure, it
might be better called is_okay, but it is barely objectionable as-is.

called isOkay or is_number_okay, then I might have issues too... but
this seems like grasping at straws.

It is typed 'bool'; its meaning can only be 'is' or 'is not'. If it was

You could have inverted the test logic for the each test and written them as separate early exits. The success case would be a fall through. The indentation level would be the same, but the code would be clearer.

I agree with explicit NULL test, particular in contexts where the

there so it would not be confusing to any developer for long.

variable declaration is not present. However, the declaration is right

Perhaps, but considering I got it wrong in this case, it certainly is confusing.

 Further,

problems to expect that I would get everything right.

I copied the core of this code from elsewhere in the tree, and I cannot manage to fix every problem in the tree in one pass. There are too many

Didn't say I expected you to.  Just pointing it out.

Finally, both of
the uses of '!' are _not_ NULL pointer checks; they are integer tests
for the end of the string.  So, yes, those checks will _always_ be 0.

Well, those _shouldn't_ be integer tests, they should be character tests. You are looking for the NULL termination on the string which is technically '\0'.

Cheers,

Zach

smime.p7s
Description: S/MIME cryptographic signature

_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development