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
> incorporating multiple logical (as in separate tests, not specifically
> those using logic operators) tests into a single statement. If they
> are going to be, then I prefer the unnecessarily parentheses be used
> 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
understand exactly what it does. OpenOCD's style should _not_ cater to
those "newer to programming". This is not a beginner's project, nor
should developers working on it expect to produce code at that level.
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?
> > If removing braces from single-line statements breaks something, then
> > the statement itself was broken (e.g. a macro lacking do { } while(0)
> > wrap) or something else needs to be fixed.
> >
>
> While certainly true, it doesn't mean that we _need_ to remove all
> extra braces. Sometimes they are for clarity rather than functionality.
I agree. You will find that many of my recent patches retain braces for
single statements when they extend across multiple lines. Likewise for
control flow "statements" like 'for', when their body flows similarly.
That said, they add needless lines when used with single-line
statements. A small function quickly balloons up ridiculously.
> >> 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>
> >
> > Seriously? You think that my efforts have increased the obfuscation?
> > My principle aim has been to improve the readability of the code, so I
> > have to wonder whether others agree with this particular assessment.
> >
>
> 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
> to handle the error cases returned by stroul. Without seeing the rest
> 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
> log. That isn't immediately clear even though operator precedence and
> 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
> usually be changed to only need a single if. The variable declaration
> 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. ;)
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.
It is typed 'bool'; its meaning can only be 'is' or 'is not'. If it was
called isOkay or is_number_okay, then I might have issues too... but
this seems like grasping at straws.
I agree with explicit NULL test, particular in contexts where the
variable declaration is not present. However, the declaration is right
there so it would not be confusing to any developer for long. Further,
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
problems to expect that I would get everything right. 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.
Cheers,
Zach
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
