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

Hi Paul,

On 9/22/2014 2:20 PM, Paul E Bennett wrote:
> 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.

Yes, but when you start slicing a project up into "manageable chunks"
to examine individually, it takes a fair bit of *concerted* effort to
remember what the roles of each of the modules you've already reviewed
were -- especially any assumptions that you made IN those reviews

IME, it's the assumptions in which bugs lie -- all the unspoken things
that you are likely to assume ONE WAY when looking at THIS; then equally
likely to assume some OTHER way when looking at THAT!  If you forget
how your internal bias resolved those assumptions at any point, you
leave yourself open to a bug slipping in under your nose.

I can't *count* the number of times I have crashed someone's code
and been met with:
    "What did you do?"
    <yada_yada_yada>
    "But, you're not SUPPOSED to do that!  That's stupid!!"
    "Then, why did you LET ME?"
    <mumble>  "Well, I assumed..."
    "What?  You assumed the bank robber wouldn't walk into an unlocked vault?"

First thing to do in any project (hardware or software) review is to
identify all the unspoken assumptions that the developer(s) made.
And, then, VIOLATE those as you examine the implementation.

E.g., I will deliberately choose a do-loop over a while-loop when I
am *sure* there will be at least one iteration (assuming "for" is
not appropriate).  This is my way of signaling that this code
*will* be executed -- at least once!  I then preface the code with
commentary and suitable invariants that explains why this is so.

As a result, those instances where I allow ZERO iterations stand
out and let others ask "what happens if this whole block gets
skipped over?  what might break??" (as that is a likely source
of errors)

[the "do-loops" are then implicitly known to NOT be skipped!]

Familiarity with each other's design/coding styles can make these
sorts of things a lot easier to sort out.  When I design hardware,
I am a big fan of FSM's.  So, folks already know what to expect
when they examine one of my hardware implementations:  clock is
king!  make sure to explore skews between clocks!!

On 9/22/2014 4:15 PM, Don Y wrote:
> 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.

Grrr... s.b., "!0".  <frown>  Too early in the day...

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

On 9/22/2014 4:24 PM, John Speth wrote:
> 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).

+42

At the very least, it shows (yourself and others) that, at one point, you
thought about why it was "OK" for this code to be expressed in this manner.
That you haven't carelessly lost sight of the fact that you've got an apple
and a Buick and are trying to make them co-operate in some way!

If you are doing this a LOT, then you need to rethink why there is such
a need.  Have you chosen the wrong representation for <something>?

On 9/22/2014 7:28 PM, Don Y wrote:
>
> Yes, but when you start slicing a project up into "manageable chunks"
> to examine individually, it takes a fair bit of *concerted* effort to
> remember what the roles of each of the modules you've already reviewed
> were -- especially any assumptions that you made IN those reviews

There is a thing called requirements allocation that deals with imposing 
specific requirements on specific modules.  Then new requirements that 
only apply to lower level entities are created *and* documented so the 
person designing the lower modules knows how to make their code play 
well with the other modules.

This is not so hard, but it has to be rigorous.


> IME, it's the assumptions in which bugs lie -- all the unspoken things
> that you are likely to assume ONE WAY when looking at THIS; then equally
> likely to assume some OTHER way when looking at THAT!  If you forget
> how your internal bias resolved those assumptions at any point, you
> leave yourself open to a bug slipping in under your nose.

There should be *no* assumptions.  Assumptions are written down and that 
means they are part of the spec and not assumptions.

...snip multiple paragraphs that say what I just wrote above...

> Familiarity with each other's design/coding styles can make these
> sorts of things a lot easier to sort out.  When I design hardware,
> I am a big fan of FSM's.  So, folks already know what to expect
> when they examine one of my hardware implementations:  clock is
> king!  make sure to explore skews between clocks!!

What are you designing that the clock has skew enough to matter?  Clock 
skew is dealt with as that, clock skew.  It is minimized enough that it 
never affects a design.  Why would you need to consider it any further? 
  The impact of too much clock skew is always a problem.

-- 

Rick

On 9/22/2014 6:59 PM, Don Y wrote:
> 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.

As a general rule this is nonsense.  Drawings are great for the things 
drawing are good for.  But when a system gets very complex you can't put 
enough detail in a top level drawing for it to be worth a damn.  The 
decomposition is important.

There is nothing wrong with drawings.  They are great where they work. 
But to say that means it has to be hardware is silly.  Software can 
utilize drawings as well.


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

More nonsense.  I have personally used a number of such tools.


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

Unsubstantiated nonesense.

> *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.

Only *simple* things can be abstracted simply.  It has nothing to do 
with hardware vs. software.


>      "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!"

What does this have to do with hardware vs. software?  Both hardware and 
software have modules.  I can't even tell which this is about!

-- 

Rick

On 9/22/2014 7:24 PM, John Speth wrote:
> 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).

I think they call that Ada... no?  I know VHDL won't let you do anything 
without type matches.

-- 

Rick

On Mon, 22 Sep 2014 12:18:17 -0700, Paul Rubin wrote:

> 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 ?

Yes!  That's why I'm walking around with an air of offended innocence!

> The error checking in GCC is much more through than the traditional lint
> programs. 

Yes, I keep hearing that.  I can only reiterate my rhetorical question!

> You might also try the Clang static analyzer. 

Sigh.  That's a thought.

> But if you're doing critical systems in C, you really should consider
> fancier tools like Coverity.

It _is_ a flight-critical system.  But it's flight-critical for a model 
airplane that flies on strings, so it's not like it's going to injure 
anyone if it malfunctions.

-- 
www.wescottdesign.com

On Tue, 23 Sep 2014 00:27:08 -0500, tim <tim@seemywebsite.com> wrote:

>On Mon, 22 Sep 2014 12:18:17 -0700, Paul Rubin wrote:
>
>> 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 ?
>
>Yes!  That's why I'm walking around with an air of offended innocence!
>
>> The error checking in GCC is much more through than the traditional lint
>> programs. 
>
>Yes, I keep hearing that.  I can only reiterate my rhetorical question!


Well, FSVO "traditional".  I'm sure it's at least as good as what
you'd find in a 1980 vintage lint...  ;-)