Re: [Nmh-workers] 1.7 release date

[Ralph Corderoy]

Hi Lyndon,

> This is code rewrite for religious purposes

Here's a rewrite example.

    -    size_t len = sizeof "user=" - 1
    -        + strlen(user)
    -        + sizeof "\1auth=Bearer " - 1
    -        + strlen(cred->access_token)
    -        + sizeof "\1\1" - 1;
    -    free(cred->ctx->sasl_client_res);
    -    cred->ctx->sasl_client_res = mh_xmalloc(len + 1);
    -    *res_len = len;
    -    sprintf(cred->ctx->sasl_client_res, "user=%s\1auth=Bearer %s\1\1",
    -            user, cred->access_token);
    -    return cred->ctx->sasl_client_res;

    +    char **p;
    +
    +    p = &cred->ctx->sasl_client_res;
    +    free(*p);
    +    *p = concat("user=", user, "\1auth=Bearer ", cred->access_token, 
"\1\1", NULL);
    +    *res_len = strlen(*p);
    +    return *p;

concat() already existed and was widely used;  not my doing.  There's a
slight runtime hit on strlen of those constants rather than sizeof, but
as someone who reads source code I prefer that over having to manually
compare the size calculation to the formatting every time;  all those -1
and +1, what's including a NUL and what isn't.  And is it always ctx in
cred that's accessed, and sasl_client_res within that, or a similar
named member, either deliberately or by error?

If rewriting to make it markedly more clear is religous then you're
correct.  And a religion most programmers follow.  It's not what I've
long understood by the term.

The changes are typically one old line for one new.  I mechanically find
candidates and then semi-mechanically change those after inspection,
e.g. to see if the data flow means an assumption can be made.
http://git.savannah.gnu.org/cgit/nmh.git/commit/?id=c8773dc680d5ec95ad5f862d44ce15124c95146a
is one example.  It's axing some of the getcpy()s, as docs/TODO has said
since at least 2000.  By looking at existing tests of a variable, or
examining its origin, or where it must pass through next, I can infer
it's not going to be NULL and thus getcpy() can become mh_xstrdup() that
doesn't take NULL.

> How are we going to validate all these memory/buffer/string related
> changes to ensure they have not introduced NEW bugs?  

Well, you could start by looking at the changes to see they're not large
and drastic like you assume.  They're mostly small and isolated.  Bigger
ones are one-off bug fixes.

> Ralph, what is your plan for code verification of these changes you
> are making?

About the same as my stand-in for getline(3), which you didn't complain
about despite it being much more complex logic than anything I've done
since.  Eyeballs and the matter behind them.  I'm careful in examining
each case before the edit.  I do it all again before committing the
change.

I get paid to inspect embedded C for theoretical errors, above what a
modern compiler or linter throws up, so I'm used to plodding through
character by character thinking about error cases.  It also means I
don't like patterns that are, with that experience, more error prone
over the longer term, e.g. become erroneous with less bit rot.  Or ones
that have a higher cognitive overhead than necessary making it harder to
maintain a full picture and thus miss higher-level problems because the
nitty-gritty has caused a brain flush.  I still don't think that's
religious, and even review for fun sometimes, e.g.
http://www.tarsnap.com/bounty-winners.html

Obviously I may make the odd mistake, but I expect there's a few of us
heavy users who are running git's version so it's getting some exercise.

> The current regression tests can't come anywhere near dealing with
> this.

They do a good job, actually, by the time some string has made it all
the way through from disk to screen, for example, and I'm very grateful
to whoever made the effort.  They been useful for the odd bug-fix
changes done by hand where the change is more complex.

-- 
Cheers, Ralph.
https://plus.google.com/+RalphCorderoy