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

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.

class CPort
{
...
  bool elapsed()          {return _elapsed;}
...
}

...

CPort _port;

...

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

-- 

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

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?

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

On 2014-09-21, Clifford Heath <no.spam@please.net> 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?
>
> This is why we should be using a language with a proper type system.

I've just encountered the exact same thing this evening, with unsigned
long int being silently truncated to unsigned char without a warning.

I needed to extend a return value from unsigned char to unsigned long
int after a spec revealed an additional usage case. I knew there was
additional code which needed changing but I did a quick compile just
to check for syntax errors in the changes to that point and the damn
thing compiled without a single warning. :-(

And I agree, you wouldn't get that past an Ada compiler.

Simon.

PS: And yes, -Wall -Werror was enabled.

-- 
Simon Clubley, clubley@remove_me.eisner.decus.org-Earth.UFP
Microsoft: Bringing you 1980s technology to a 21st century world

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?
> 
> This is why we should be using a language with a proper type system.

I use a language that has no sense of typing and can manage to produce 
correct code without too much fuss.

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.

-- 
********************************************************************
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 22/09/14 10:21, Simon Clubley wrote:
> On 2014-09-21, Clifford Heath <no.spam@please.net> 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)
>> This is why we should be using a language with a proper type system.
> And I agree, you wouldn't get that past an Ada compiler.

Ada's type system is strong, but terribly intrusive, worse even than 
Java (which isn't even strongly-typed). It's that kind of intrusiveness 
that drives folk like Paul away from typed languages - though those have 
their place; I like Ruby, with high-coverage BDD.

Modern languages like Haskell implement strong typing using type 
inferencing (you almost never need type annotations), which make them 
just as nice and succinct to write in as untyped scripting languages, 
but as safe or safer than Ada. They're becoming viable for embedded 
work, too, though I suspect more evolution is needed. Haskell is lazy, 
for example, which makes it hard to reason about performance. Rust is a 
newer contender though.

Type inferencing is the way of the future, in any case.

Clifford Heath.

On 9/21/2014 8:21 PM, Simon Clubley wrote:
> On 2014-09-21, Clifford Heath <no.spam@please.net> 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?
>>
>> This is why we should be using a language with a proper type system.
>
> I've just encountered the exact same thing this evening, with unsigned
> long int being silently truncated to unsigned char without a warning.

I don't use C++ but when I did C programming I would clean my code to 
get rid of all warnings while the other coders said I was being silly as 
the warnings were just "warnings".  lol

I thought this type of thing *would* produce a warning.  Are the 
compiler flags set to not give this sort of warning?


> I needed to extend a return value from unsigned char to unsigned long
> int after a spec revealed an additional usage case. I knew there was
> additional code which needed changing but I did a quick compile just
> to check for syntax errors in the changes to that point and the damn
> thing compiled without a single warning. :-(
>
> And I agree, you wouldn't get that past an Ada compiler.
>
> Simon.
>
> PS: And yes, -Wall -Werror was enabled.

Oh, ok.

Everyone complains about the strong typing in VHDL.  It's the problems 
you don't see because you couldn't make them that it saves you from. 
Hard to show people those things.

-- 

Rick

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.

Clifford Heath wrote:
> On 22/09/14 10:21, Simon Clubley wrote:
>> On 2014-09-21, Clifford Heath <no.spam@please.net> 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)
>>> This is why we should be using a language with a proper type system.

All languages have a "proper type system." You - yes, you , you're the
sentient creature in this relationship, presumably - just have to know 
what you're doing.

The types in 'C' just happen to have a referent to the physical types of
the hardware, usually. This is okay.

The time to complain about 'C', like the time to plant a tree, is thirty 
years ago.

Go to about 58:00 or so in this video
http://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare

Side effects; they're what's for dinner.

>> And I agree, you wouldn't get that past an Ada compiler.
>
> Ada's type system is strong, but terribly intrusive, worse even than
> Java (which isn't even strongly-typed). It's that kind of intrusiveness
> that drives folk like Paul away from typed languages - though those have
> their place; I like Ruby, with high-coverage BDD.
>
> Modern languages like Haskell implement strong typing using type
> inferencing (you almost never need type annotations), which make them
> just as nice and succinct to write in as untyped scripting languages,
> but as safe or safer than Ada. They're becoming viable for embedded
> work, too, though I suspect more evolution is needed. Haskell is lazy,
> for example, which makes it hard to reason about performance. Rust is a
> newer contender though.
>
> Type inferencing is the way of the future, in any case.
>
> Clifford Heath.


It's always thirty years in the future, and has been for at least
thirty years. I can name a dozen solutions - one's still alive; Bruce 
Baden Powell - oops, Bruce Powell Douglass' use of executable UML.
Its darned close to provably correct if you're strenuous enough
about it.

Nobody uses it.

-- 
Les Cargill

On Sun, 21 Sep 2014 20:16:47 -0700, Don Y <this@is.not.me.com> 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.

Good topic for airing from a number of angles. I'm part of a very
small team developing specialised code for large manufacturing plants,
using arcane languages and platforms that mean that an 'ordinary'
programmer isn't going to be of much help.  I've made the point to our
team leaders that in the wider embedded software industry, code
overview/reviewing is a strongly recommended practice, but for the
usual reasons, getting anyone assigned to the job never happens.

The few times I've managed to get a colleague, unofficially, to do a
review for me, the results haven't been great.  It's relatively easy
to catch the superficial stuff, such as pointless looping and crappy
naming. But the things that cause trouble are embedded much deeper,
and catching them would require a thorough understanding of the
situation - a programmer-to-programmer brain transplant almost - to
achieve much.  I'd be interested in how this is dealt with by others.
How counterproductive is lack of extreme detail in the functional
specification? Is there such a thing as a methodology that people
would recommend?  Is top-down or bottom up better (and is this
question meaningful?). Is it possible to estimate review time per
line, or on some other basis?  

On 9/21/2014 8:51 PM, "Bruce Varley" wrote:

> Good topic for airing from a number of angles. I'm part of a very
> small team developing specialised code for large manufacturing plants,
> using arcane languages and platforms that mean that an 'ordinary'
> programmer isn't going to be of much help.

You'd be surprised at how many parallels pertain to other industries,
regardless of languages, platforms, etc.

I.e., application domains tend to be fairly specialized (unless you're
writing desktop code -- though even there!).  A person developing
motor control code and another developing software to analyze blood
samples have very little common ground (besides, perhaps, a language).

> I've made the point to our
> team leaders that in the wider embedded software industry, code
> overview/reviewing is a strongly recommended practice, but for the
> usual reasons, getting anyone assigned to the job never happens.

IME, you can't treat it as a checkoff item that happens at a
specific place(s) in the development process.  The results are
much better if your colleagues are intimately aware of the
project, have discussed aspects of it with you in the past, etc.

At the very least, they feel less like "outsiders" and more
"invested" in the (your) project.

One place I worked had token rewards (trivial things -- a candy bar!)
to acknowledge value contributed by others to "your" project.  Sounds
silly but it makes the event more memorable to all parties.

> The few times I've managed to get a colleague, unofficially, to do a
> review for me, the results haven't been great.  It's relatively easy
> to catch the superficial stuff, such as pointless looping and crappy
> naming. But the things that cause trouble are embedded much deeper,
> and catching them would require a thorough understanding of the
> situation - a programmer-to-programmer brain transplant almost - to
> achieve much.

Exactly.  Doing this after the fact is like proofreading a novel:
yeah, you might catch some spelling, punctuation, grammatical
errors -- but, any comments you have on the storyline are too late
to be effective (then why bother offering them?).

OTOH, if you'd been discussing your "plot line" with that future
proofreader all along, you might have been able to benefit from
these observations early on.

> I'd be interested in how this is dealt with by others.
> How counterproductive is lack of extreme detail in the functional
> specification?

I find a detailed specification makes things a LOT easier to
critique.  I now write user manuals early on as these let
folks (clients, potential users, myself, etc.) envision REAL
situations and how they will be handled.  It's amazing
how many big holes you can find in a client's proposal once
you start rewriting it in this form ("gee, we have a way of
CREATING foobars but no way of DELETING them!  Oh, and, by
the way, how many of these can we create?  And, how does this
affect the usability of the system?")

> Is there such a thing as a methodology that people
> would recommend?

I spend a lot of time documenting exceptions and special cases
in my source commentary.  "I'm doing it this way BECAUSE..."

In recent projects, I am removing this from the codebase and,
instead, preparing "tutorials" -- essentially, organizing my
working notes in a manner that allows others to see how I
came to a particular decision (instead of just documenting
the decision itself).

This also helps identify the cases that *need* to be verified
when the code is tested.  And, lets others examine the test
suite to consider other situations that don't appear, there.

> Is top-down or bottom up better (and is this
> question meaningful?). Is it possible to estimate review time per
> line, or on some other basis?

I've never seen any productive results doing "line by line" style
reviews.  People QUICKLY "zone-out".  It's too tedious and yields
too few "hits".  Rather, indicate WHAT you are trying to do and HOW
you are doing it.  Let your reviewers ASSUME you have implemented
this correctly (why examine every line of code?).  Indicate how
you have tested it (did you forget some boundary condition?).
Then, let them think about what COULD go wrong with your overall
approach -- and YOU explain why that's NOT a problem...

"What happens if we want to create 5,000 foobars?  How would a user
manage all of those 'objects'/names?"