D'oh! Oh, for lack of a code review.

On 21/09/14 23:23, Tim Wescott wrote:
> I just had to share this, god knows why.  I keep thinking that I want to 
> figure out a way to gather up a group of five or ten embedded software 
> consultants in my area and form a code review exchange, to keep things 
> like this from happening.  See if YOU can find the error!
> 
> I'm not sure why the Gnu compiler bitches if I compare an int to an 
> unsigned int, but doesn't bitch when I compare a boolean to an int -- but 
> there you are.
> 

As always in the software world, "we'll fix that in the next version" :

<https://gcc.gnu.org/gcc-5/changes.html>


-Wbool-compare (activated by -Wall) should spot that, but unless you
like the bleeding edge, you'll probably want to wait for the the stable
release.

> I'm not sure why the Gnu compiler bitches if I compare an int to an
> unsigned int,

Because that has partly unspecified results: what is the result of 
comparing -1 to 65535U on a 16-bit twos-complement machine? A 32-bit 
one? And how is the compiler to assume that's what you wanted?

> but doesn't bitch when I compare a boolean to an int

It wouldn't have anything to complain about: that has a perfectly 
well-defined result.

> if (_port.elapsed() > 0 &&
> 	_port.elapsed() < ((_port.clockHz() * 50LL) / 1000000))

Any self-respecting code reviewer still awake at this point of the 
procedure would have rapped you over the knuckles here for using a 
boolean to do what is rather obviously non-boolean work.

But since this is a _design_ error, not a _coding_ mistake, it's a bit 
too much to expect a compiler to warn you about it.  Mind-reading still 
isn't in the requirements spec for a compiler.

But I believe a properly tuned Lint could have caught this (particularly 
if it's been set to enforce MISRA C, which does frown upon such things).

On Mon, 22 Sep 2014 19:08:58 +0200, Hans-Bernhard Bröker wrote:

>> I'm not sure why the Gnu compiler bitches if I compare an int to an
>> unsigned int,
> 
> Because that has partly unspecified results: what is the result of
> comparing -1 to 65535U on a 16-bit twos-complement machine? A 32-bit
> one? And how is the compiler to assume that's what you wanted?
> 
>> but doesn't bitch when I compare a boolean to an int
> 
> It wouldn't have anything to complain about: that has a perfectly
> well-defined result.
> 
>> if (_port.elapsed() > 0 &&
>> 	_port.elapsed() < ((_port.clockHz() * 50LL) / 1000000))
> 
> Any self-respecting code reviewer still awake at this point of the
> procedure would have rapped you over the knuckles here for using a
> boolean to do what is rather obviously non-boolean work.
> 
> But since this is a _design_ error, not a _coding_ mistake, it's a bit
> too much to expect a compiler to warn you about it.  Mind-reading still
> isn't in the requirements spec for a compiler.
> 
> But I believe a properly tuned Lint could have caught this (particularly
> if it's been set to enforce MISRA C, which does frown upon such things).

This is actually a bitch I have about the gnu tools team.  Their take on 
Lint is "Oh, you don't need Lint, the error-checking in gnu is plenty good 
enough".

Well if gnu-C can be made so lint-like, why didn't it catch this, then!

I would call it a coding error, but I think it's just semantics: from the 
bits of code that I _didn't_ share, it's pretty obvious that the intent of 
the function call was to share an integer value: turning it into a boolean 
was a mistake, and was probably just a typographical error on my part.

I wouldn't expect a compiler to mind read.  I would, however, expect a 
compiler from a team that claims that it replaces Lint to allow error 
checking around such clear errors of intent as treating booleans like 
integers and visa-versa.  (I know -- this goes against the C++ philosophy 
of "lots of rope, and a few pre-tied hangman's nooses in the STL".  But we 
don't need to prove our manhood EVERY SINGLE DAY, do we?)

-- 

Tim Wescott
Wescott Design Services
http://www.wescottdesign.com

Tim Wescott <seemywebsite@myfooter.really> writes:
> Well if gnu-C can be made so lint-like, why didn't it catch this, then!

Did you use -Wall ?

The error checking in GCC is much more through than the traditional lint
programs.  You might also try the Clang static analyzer.  But if you're
doing critical systems in C, you really should consider fancier tools
like Coverity.

On 22/09/14 20:59, Tim Wescott wrote:
> On Mon, 22 Sep 2014 19:08:58 +0200, Hans-Bernhard Bröker wrote:
>
>>> I'm not sure why the Gnu compiler bitches if I compare an int to an
>>> unsigned int,
>>
>> Because that has partly unspecified results: what is the result of
>> comparing -1 to 65535U on a 16-bit twos-complement machine? A 32-bit
>> one? And how is the compiler to assume that's what you wanted?
>>
>>> but doesn't bitch when I compare a boolean to an int
>>
>> It wouldn't have anything to complain about: that has a perfectly
>> well-defined result.
>>
>>> if (_port.elapsed() > 0 &&
>>> 	_port.elapsed() < ((_port.clockHz() * 50LL) / 1000000))
>>
>> Any self-respecting code reviewer still awake at this point of the
>> procedure would have rapped you over the knuckles here for using a
>> boolean to do what is rather obviously non-boolean work.
>>
>> But since this is a _design_ error, not a _coding_ mistake, it's a bit
>> too much to expect a compiler to warn you about it.  Mind-reading still
>> isn't in the requirements spec for a compiler.
>>
>> But I believe a properly tuned Lint could have caught this (particularly
>> if it's been set to enforce MISRA C, which does frown upon such things).
>
> This is actually a bitch I have about the gnu tools team.  Their take on
> Lint is "Oh, you don't need Lint, the error-checking in gnu is plenty good
> enough".
>
> Well if gnu-C can be made so lint-like, why didn't it catch this, then!

They are getting there - it's in the development version of gcc.

In general, gcc catches more compile-time errors than most other C 
compilers - but it doesn't catch everything.  My brief trials with 
PC-Lint suggest that it is too noisy for normal use unless you fill your 
code with specially formatted comments - and if you also use doxygen, 
it's almost impossible to see the actual code.  And like many such 
tools, it doesn't keep up to date with the latest standards as well as 
compilers, and it doesn't follow any of the gcc extensions.

Another checker (in addition to gcc -Wall -Wextra -W...) that is looking 
quite good these days is llvm's checker.  I am thinking of taking that 
for a spin when I get the time.

For me, the compiler is the ideal place to put the error checking.  I 
would prefer the gcc team put more emphasis on this - it gets gradually 
better with each release, but I wouldn't mind some bigger steps.  Maybe 
"competition" from llvm will encourage that (it's pushed gcc to better 
LTO and better error messages).  Unfortunately for c.a.e. folks, many of 
the recent improvements have been for run-time checking and 
instrumentation to catch memory leaks, multi-threading problems and 
undefined behaviour.

>
> I would call it a coding error, but I think it's just semantics: from the
> bits of code that I _didn't_ share, it's pretty obvious that the intent of
> the function call was to share an integer value: turning it into a boolean
> was a mistake, and was probably just a typographical error on my part.
>
> I wouldn't expect a compiler to mind read.  I would, however, expect a
> compiler from a team that claims that it replaces Lint to allow error
> checking around such clear errors of intent as treating booleans like
> integers and visa-versa.  (I know -- this goes against the C++ philosophy
> of "lots of rope, and a few pre-tied hangman's nooses in the STL".  But we
> don't need to prove our manhood EVERY SINGLE DAY, do we?)
>

Don Y wrote:

> On 9/21/2014 5:28 PM, Paul E Bennett wrote:
>> I like Tim's idea of forming Code Review Circles, which is one way of
>> keeping the "Lone Wolf Developer" from going too insane. Fortunately I
>> have a few friends that will help review material for me (documents as
>> well as code and drawings). I do similar for them too. It is part of what
>> networking is all about.
> 
> I think this is only practical in "dense" areas -- and, only with
> contractors. Folks who have 9-to-5's tend to want to leave work behind at
> 5 (and would probably not be compensated to do this sort of thing on the
> company's dime!)
> 
> You can do this "long distance" but in my experiences, it loses a lot of
> the intimacy and energy that "in person" reviews seem to exhibit.  And,
> a lot more gets overlooked (because of the "cost" of bringing someone's
> attention to an issue -- wondering if everyone else has already made a
> similar observation).
> 
> It's *really* nice to have a bunch of heads looking at the *same* thing...
> knowing where each other's gaze/focus is drifting, etc.
> 
> It's a LOT easier to do hardware reviews remotely.  Especially with
> interactive video, etc.  "Everything" is visible at one time -- to
> everyone, etc.

I would never suggest having a whole software project dumped on the reviewer 
in one go. However, if you follow the rules about creating readable code, 
decompose functionality and partition well enough you should be able to get 
small bits reviewed no matter how remote your review partner is. 

They need sight of the (part) specification that led to your design/code so 
that they can draw their own conclusions about whether you were on the right 
track or were making things over complex.

-- 
********************************************************************
Paul E. Bennett IEng MIET.....<email://Paul_E.Bennett@topmail.co.uk>
Forth based HIDECS Consultancy.............<http://www.hidecs.co.uk>
Mob: +44 (0)7811-639972
Tel: +44 (0)1235-510979
Going Forth Safely ..... EBA. www.electric-boat-association.org.uk..
********************************************************************

On 9/21/2014 11:16 PM, Don Y wrote:
> On 9/21/2014 5:28 PM, Paul E Bennett wrote:
>> I like Tim's idea of forming Code Review Circles, which is one way of
>> keeping the "Lone Wolf Developer" from going too insane. Fortunately I
>> have
>> a few friends that will help review material for me (documents as well as
>> code and drawings). I do similar for them too. It is part of what
>> networking
>> is all about.
>
> I think this is only practical in "dense" areas -- and, only with
> contractors.
> Folks who have 9-to-5's tend to want to leave work behind at 5 (and would
> probably not be compensated to do this sort of thing on the company's
> dime!)
>
> You can do this "long distance" but in my experiences, it loses a lot of
> the intimacy and energy that "in person" reviews seem to exhibit.  And,
> a lot more gets overlooked (because of the "cost" of bringing someone's
> attention to an issue -- wondering if everyone else has already made a
> similar observation).
>
> It's *really* nice to have a bunch of heads looking at the *same* thing...
> knowing where each other's gaze/focus is drifting, etc.
>
> It's a LOT easier to do hardware reviews remotely.  Especially with
> interactive video, etc.  "Everything" is visible at one time -- to
> everyone, etc.

These days hardware *is* software.  The last company I worked for had 
the FPGA people in the software department.

-- 

Rick

On 9/22/2014 3:48 PM, rickman wrote:
> On 9/21/2014 11:16 PM, Don Y wrote:
>> On 9/21/2014 5:28 PM, Paul E Bennett wrote:
>>> I like Tim's idea of forming Code Review Circles, which is one way of
>>> keeping the "Lone Wolf Developer" from going too insane. Fortunately I
>>> have
>>> a few friends that will help review material for me (documents as well as
>>> code and drawings). I do similar for them too. It is part of what
>>> networking
>>> is all about.
>>
>> I think this is only practical in "dense" areas -- and, only with
>> contractors.
>> Folks who have 9-to-5's tend to want to leave work behind at 5 (and would
>> probably not be compensated to do this sort of thing on the company's
>> dime!)
>>
>> You can do this "long distance" but in my experiences, it loses a lot of
>> the intimacy and energy that "in person" reviews seem to exhibit.  And,
>> a lot more gets overlooked (because of the "cost" of bringing someone's
>> attention to an issue -- wondering if everyone else has already made a
>> similar observation).
>>
>> It's *really* nice to have a bunch of heads looking at the *same* thing...
>> knowing where each other's gaze/focus is drifting, etc.
>>
>> It's a LOT easier to do hardware reviews remotely.  Especially with
>> interactive video, etc.  "Everything" is visible at one time -- to
>> everyone, etc.
>
> These days hardware *is* software.  The last company I worked for had the FPGA
> people in the software department.

You can far more easily represent a piece of hardware on a "unified drawing"
than you can with software.  So, you can provide a "block/functional diagram"
that allows everyone to see the overall structure of a circuit; then, delve
into selected "subsections" as appropriate.

Tools (and "thought processes") for doing this with a piece of software
just don't exist in widespread use.

This is why software is considerably harder to document, describe, analyze
(in an isolated human brain), etc.

*When* adequate tools (and methodologies) exist to provide (and are adopted)
these same sorts of "illustrations" that allow entire software systems to be
"conceptualized" in such a terse, abstract manner, I suspect there will be
a quantum leap in software reliability, etc.

     "Why is this module talking to this *other* module?  Is that intentional?
     It *smells* like a potential problem area... let's take a look inside
     those two modules!"

On 9/21/2014 4:02 PM, Clifford Heath wrote:
> On 22/09/14 07:23, Tim Wescott wrote:
>> See if YOU can find the error!
>> if (_port.elapsed() > 0 &&
>>     _port.elapsed() < ((_port.clockHz() * 50LL) / 1000000))
>
> 0 and 1 will always be less than ((_port.clockHz() * 50LL) / 1000000)
> Do I get a cookie?

Actually, there are more fundamental bugs.  Was the OP's intent to invoke
the "elapsed" method on "port" *once* or *twice*?  Or, twice iff the
first invocation yields 0?  Side-effects?  etc.

> This is why we should be using a language with a proper type system.

On 9/21/2014 2:23 PM, Tim Wescott wrote:
> I just had to share this, god knows why.  I keep thinking that I want to
> figure out a way to gather up a group of five or ten embedded software
> consultants in my area and form a code review exchange, to keep things
> like this from happening.  See if YOU can find the error!

It's a nice challenge to find the bug however the real bug is in poor 
code expression.  I would love it if a C/C++ compiler would reject those 
kinds of type mismatches.  A good lint tool should flag it.

My practice is to explicitly acknowledge type mismatches in the code to 
improved readability (like casting).

JJS