Style (?) issue

Hi,

(sigh)  Another *@&^#($ night of "tree sitting".  This gets
old REALLY FAST!  :-/

Anyway, between trips outside, I've been dressing up
some old bits of software -- cleaning up commentary, etc.

One of the things I do to help with debugging as well as
making interfaces more explicit is to place DEBUG-only
invariants at the start of each function that "verify"
the terms of the invocation contract.  E.g., if a pointer
can't be NULL, I'll do something like:

ASSERT( (src != NULL), "Pointer to source must be non-NULL" );

At run time, these go away (usually... depends on the market
I am serving).  So, they don't "cost anything".  And, they
make the interface rules *very* explicit (while the
commentary could possibly be ambiguous, the conditionals
in the invariants are hard to argue with!).

So, the start of each function is usually a list of several
ASSERT()'s -- at least one for each parameter (usually).
Then, the meat of the function begins.

One of the routines I was dressing up today had some constraints
placed on a pointer passed to it.  In particular, requiring the
pointer to reference a valid memory location in a certain
range (defined by manifest constants).

ONCE THROUGH THE INVARIANTS, "live code" would further qualify
the parameters passed to the routine.  In this case, enforcing
some "arbitrary" alignment criteria.  Conceivably, the aligned
pointer could now reference something *outside* the range of
memory locations enforced by the invariants.  The code clearly
has to test for this -- since the whole point is to ensure
those "out of bounds" areas are not referenced at run-time.

Now, the nit pick... *technically*, the caller has complied
with the contractual requirements of the function (the contract
doesn't enforce alignment issues -- that is part of the role
of the function itself). As such, I maintain that the original
set of invariants are *all* that should be enforced (to
determine if the invocation "complies").  The *further*,
non-contractual alignment restriction that the function imposes
should be treated as a run-time error -- just like trying to
resolve  "ptr[index]" when the location referenced extends
outside of the valid range.

In other words, I should *not* include a second set of ASSERT's
to examine the "aligned" parameters after their initial validation.

Agreed?  Else, where does it end??  (it is really "clean" having
the invariants clumped in one cohesive group)

Thx,
--don

On 1/3/2011 2:50 AM, D Yuniskis wrote:
> Now, the nit pick... *technically*, the caller has complied
> with the contractual requirements of the function (the contract
> doesn't enforce alignment issues -- that is part of the role
> of the function itself). As such, I maintain that the original
> set of invariants are *all* that should be enforced (to
> determine if the invocation "complies"). The *further*,
> non-contractual alignment restriction that the function imposes
> should be treated as a run-time error -- just like trying to
> resolve "ptr[index]" when the location referenced extends
> outside of the valid range.

Sorry, I should clarify that routines typically return
status of:
   SUCCESS
   FAILURE
   TIMEOUT
   QUALIFIED
with the last being "success but with modified arguments"
(which would be signaled, in this case, where the pointer is
modified to enforce alignment constraints *if* it still
fell within the "valid" range; "FAILURE" if the aligned
pointer was outside the range)

On Mon, 03 Jan 2011 02:50:07 -0700, D Yuniskis wrote:

> Now, the nit pick... *technically*, the caller has complied with the
> contractual requirements of the function (the contract doesn't enforce
> alignment issues -- that is part of the role of the function itself). As
> such, I maintain that the original set of invariants are *all* that
> should be enforced (to determine if the invocation "complies").  The
> *further*, non-contractual alignment restriction that the function
> imposes should be treated as a run-time error -- just like trying to
> resolve  "ptr[index]" when the location referenced extends outside of
> the valid range.
> 
> In other words, I should *not* include a second set of ASSERT's to
> examine the "aligned" parameters after their initial validation.
> 
> Agreed?  Else, where does it end??  (it is really "clean" having the
> invariants clumped in one cohesive group)

Putting an ASSERT in the code has a couple of benefits:

- Damage control (an ASSERT may be better than unpredicted result from 
unchecked code)

- Reducing number of bugs in final product

- More information during testing/debug, saving time.

There are also some disadvantages:

- More resources used (development time, execution time, code space)

- Cluttering of the main code path with noise

- Extra bugs due to bad asserts.

For each ASSERT, you'll have to weigh the advantages against the 
disadvantages, and multiply by chance that you'll actually hit that 
assert.

On 03/01/2011 12:46, Arlet Ottens wrote:
> On Mon, 03 Jan 2011 02:50:07 -0700, D Yuniskis wrote:
>
>> Now, the nit pick... *technically*, the caller has complied with the
>> contractual requirements of the function (the contract doesn't enforce
>> alignment issues -- that is part of the role of the function itself). As
>> such, I maintain that the original set of invariants are *all* that
>> should be enforced (to determine if the invocation "complies").  The
>> *further*, non-contractual alignment restriction that the function
>> imposes should be treated as a run-time error -- just like trying to
>> resolve  "ptr[index]" when the location referenced extends outside of
>> the valid range.
>>
>> In other words, I should *not* include a second set of ASSERT's to
>> examine the "aligned" parameters after their initial validation.
>>
>> Agreed?  Else, where does it end??  (it is really "clean" having the
>> invariants clumped in one cohesive group)
>
> Putting an ASSERT in the code has a couple of benefits:
>
> - Damage control (an ASSERT may be better than unpredicted result from
> unchecked code)
>
> - Reducing number of bugs in final product
>
> - More information during testing/debug, saving time.
>
> There are also some disadvantages:
>
> - More resources used (development time, execution time, code space)
>
> - Cluttering of the main code path with noise
>
> - Extra bugs due to bad asserts.
>
> For each ASSERT, you'll have to weigh the advantages against the
> disadvantages, and multiply by chance that you'll actually hit that
> assert.
>

One other thing you can use is static asserts.  The only disadvantage 
these have is cluttering the code, but it is not much worse than the 
typical comment (i.e., you write "static_assert(sizeOfBuffer > 100);" 
rather than "/* sizeOfBuffer should be at least 100 */".

Of course, until static asserts get fully ratified in the various C and 
C++ standards, and then get implemented in compilers, then you have to 
make them yourself with some messy macros.  But the resource usage is zero.

On 03/01/2011 10:50, D Yuniskis wrote:
> Hi,
>
> (sigh) Another *@&^#($ night of "tree sitting". This gets
> old REALLY FAST! :-/
>
> Anyway, between trips outside, I've been dressing up
> some old bits of software -- cleaning up commentary, etc.
>
> One of the things I do to help with debugging as well as
> making interfaces more explicit is to place DEBUG-only
> invariants at the start of each function that "verify"
> the terms of the invocation contract. E.g., if a pointer
> can't be NULL, I'll do something like:
>
> ASSERT( (src != NULL), "Pointer to source must be non-NULL" );
>
> At run time, these go away (usually... depends on the market
> I am serving). So, they don't "cost anything". And, they
> make the interface rules *very* explicit (while the
> commentary could possibly be ambiguous, the conditionals
> in the invariants are hard to argue with!).
>

If your compiler supports checking these sorts of things directly, then 
it is a good idea to do so (via macros if necessary to handle 
portability issues).  If you are using some sort of lint program, then 
make use of that too.  This will give you better compile-time checking, 
without run-time costs.  If the compiler can do such checking, then it 
can even give run-time benefits as the compiler may be able to make more 
optimal code.

For example, with gcc you would write:

extern void foo(unsigned char *src) __attribute((nonnull(1)));

That way the compiler will never knowingly call the function with a null 
pointer.

Of course, you would want to check the exact functionality of such 
checks, and whether you might get false positives or false negatives.


> So, the start of each function is usually a list of several
> ASSERT()'s -- at least one for each parameter (usually).
> Then, the meat of the function begins.
>

Where possible, you want to do the checks /before/ the function is 
called.  If the function is called with incorrect parameters, it is the 
/caller/ that is wrong.  Identifying the error at run-time in the called 
function may be better than nothing, but it is not nearly as good as 
identifying it at compile-time in the caller.  If you can't put the 
requirements into the "extern" declaration (as above), consider wrapping 
the real function with an inline function:

extern void do_foo(unsigned char *src);
static inline void foo(unsigned char *src) {
	ASSERT((src != NULL), "src must be non-null");
	do_foo(src);
}

If foo() is compiled with a src that is known to be non-NULL, this 
results in zero overhead, and if it is known to be NULL, then there will 
likely be a compile-time error or warning when the caller code is compiled.

For even more advanced usage, you can use something like gcc's 
"__builtin_constant_p" function to distinguish between compile-time 
known values - this would let the compiler choose between static 
assertions and a call to a "safe" version of the function, or a call to 
a dynamic assertion version of the function (which would have the 
ASSERTs, then call the safe version).

Of course, you would want to wrap all this in some macros to hide the 
messy bits.


> One of the routines I was dressing up today had some constraints
> placed on a pointer passed to it. In particular, requiring the
> pointer to reference a valid memory location in a certain
> range (defined by manifest constants).
>
> ONCE THROUGH THE INVARIANTS, "live code" would further qualify
> the parameters passed to the routine. In this case, enforcing
> some "arbitrary" alignment criteria. Conceivably, the aligned
> pointer could now reference something *outside* the range of
> memory locations enforced by the invariants. The code clearly
> has to test for this -- since the whole point is to ensure
> those "out of bounds" areas are not referenced at run-time.
>
> Now, the nit pick... *technically*, the caller has complied
> with the contractual requirements of the function (the contract
> doesn't enforce alignment issues -- that is part of the role
> of the function itself). As such, I maintain that the original
> set of invariants are *all* that should be enforced (to
> determine if the invocation "complies"). The *further*,
> non-contractual alignment restriction that the function imposes
> should be treated as a run-time error -- just like trying to
> resolve "ptr[index]" when the location referenced extends
> outside of the valid range.
>

This I don't understand.  Either it is an error to call the function 
with an unaligned pointer, or it is not an error.  If it is an error, 
then it is part of the requirements for calling the function, and should 
be checked and enforced as soon as possible.  If it is not an error, 
then it is not an error.  The function may use alignment information to 
run faster, or it may give you some debugging information to help 
improve the code, but it is not an error.


> In other words, I should *not* include a second set of ASSERT's
> to examine the "aligned" parameters after their initial validation.
>
> Agreed? Else, where does it end?? (it is really "clean" having
> the invariants clumped in one cohesive group)
>

Yes, it is "clean" having the requirements for a function listed in one 
place (you are talking about "requirements" here, not "invariants"). 
Requirements are part of the declaration of the function, even if C 
doesn't have a way to express them beyond the type declarations.

Hi Arlet,

On 1/3/2011 4:46 AM, Arlet Ottens wrote:
> On Mon, 03 Jan 2011 02:50:07 -0700, D Yuniskis wrote:
>
>> Now, the nit pick... *technically*, the caller has complied with the
>> contractual requirements of the function (the contract doesn't enforce
>> alignment issues -- that is part of the role of the function itself). As
>> such, I maintain that the original set of invariants are *all* that
>> should be enforced (to determine if the invocation "complies").  The
>> *further*, non-contractual alignment restriction that the function
>> imposes should be treated as a run-time error -- just like trying to
>> resolve  "ptr[index]" when the location referenced extends outside of
>> the valid range.
>>
>> In other words, I should *not* include a second set of ASSERT's to
>> examine the "aligned" parameters after their initial validation.
>>
>> Agreed?  Else, where does it end??  (it is really "clean" having the
>> invariants clumped in one cohesive group)
>
> Putting an ASSERT in the code has a couple of benefits:
>
> - Damage control (an ASSERT may be better than unpredicted result from
> unchecked code)
> - Reducing number of bugs in final product
> - More information during testing/debug, saving time.
>
> There are also some disadvantages:
>
> - More resources used (development time, execution time, code space)

Implementing them as macros lets me (usually) have them
"magically disappear" from production builds (e.g.,
#ifdef DEBUG
#  define ASSERT(requirement, message) do { if (!(requirement)) { ...
#else
#  define ASSERT(requirement, message)  /* this space left blank */
#endif

So, it takes up resources when I need it to do so and not
thereafter (#undef DEBUG)

> - Cluttering of the main code path with noise

Yes.  Though, to my credit, I only check for each criteria
once.

> - Extra bugs due to bad asserts.
>
> For each ASSERT, you'll have to weigh the advantages against the
> disadvantages, and multiply by chance that you'll actually hit that
> assert.

The issue I am trying to address is not whether to add more
asserts but, rather, whether to use the same mechanism that
I use to enforce contractual obligations (instead of ASSERT,
call it CONTRACT) when I am not *technically* enforcing
contractual obligations.

I.e., once through the ASSERTs (CONTRACTs), the user has met
is contractual obligations.  Technically, any further tests
that I use/impose are now run-time checks and/or optimizations.
As such, handled as (worst case):

    if (!(condition)) {
        /* whatever */
        return FAILURE;
    }

The ASSERTs typically have much steeper consequences -- forced
termination/crash -- as they are NEVER SUPPOSED TO HAPPEN (if
the developer adhered to contractual obligations).

Hi David,

On 1/3/2011 4:58 AM, David Brown wrote:
>>> Now, the nit pick... *technically*, the caller has complied with the
>>> contractual requirements of the function (the contract doesn't enforce
>>> alignment issues -- that is part of the role of the function itself). As

>>> In other words, I should *not* include a second set of ASSERT's to
>>> examine the "aligned" parameters after their initial validation.
>>>
>>> Agreed? Else, where does it end?? (it is really "clean" having the
>>> invariants clumped in one cohesive group)
>>
>> Putting an ASSERT in the code has a couple of benefits:

>> There are also some disadvantages:

>> For each ASSERT, you'll have to weigh the advantages against the
>> disadvantages, and multiply by chance that you'll actually hit that
>> assert.
>
> One other thing you can use is static asserts. The only disadvantage
> these have is cluttering the code, but it is not much worse than the
> typical comment (i.e., you write "static_assert(sizeOfBuffer > 100);"
> rather than "/* sizeOfBuffer should be at least 100 */".
>
> Of course, until static asserts get fully ratified in the various C and
> C++ standards, and then get implemented in compilers, then you have to
> make them yourself with some messy macros. But the resource usage is zero.

This is what I currently do (hence "ASSERT()" instead of "assert()").
However, they may or may not generate code in the production release
(depending on how I define them).  Sometimes they push an error to
my "black box" and abort the function call (since the body of the
function thereafter EXPECTS never to see input parameters that
violate those stated "requirements").  Other times they may just
"vanish"

Hi David,

On 1/3/2011 5:32 AM, David Brown wrote:
>> So, the start of each function is usually a list of several
>> ASSERT()'s -- at least one for each parameter (usually).
>> Then, the meat of the function begins.
>
> Where possible, you want to do the checks /before/ the function is
> called. If the function is called with incorrect parameters, it is the
> /caller/ that is wrong. Identifying the error at run-time in the called
> function may be better than nothing, but it is not nearly as good as
> identifying it at compile-time in the caller. If you can't put the
> requirements into the "extern" declaration (as above), consider wrapping
> the real function with an inline function:
>
> extern void do_foo(unsigned char *src);
> static inline void foo(unsigned char *src) {
> ASSERT((src != NULL), "src must be non-null");
> do_foo(src);
> }
>
> If foo() is compiled with a src that is known to be non-NULL, this

This is rarely the case ------------------^^^^^^^^^^^ hence the need
for the run-time check.

> results in zero overhead, and if it is known to be NULL, then there will
> likely be a compile-time error or warning when the caller code is compiled.

The compiler can't always make decisions at compile time.
I.e., there will be (many) cases where the extra code generated
by the ASSERT() gets included in the build image.  (remember,
I do this on *every* function I write)  This really eats up
a lot of space (especially for the strings)

The advantage it has is that it "forces" this into header
files where it technically belongs (if you assume the
headers to define the interfaces).  *And*, makes it more
obvious where the "bug" lies (since I can __FILE__ &
__LINE__ to point me directly to the offending invocation
instead of having to examine the call trace)

> For even more advanced usage, you can use something like gcc's
> "__builtin_constant_p" function to distinguish between compile-time
> known values - this would let the compiler choose between static
> assertions and a call to a "safe" version of the function, or a call to
> a dynamic assertion version of the function (which would have the
> ASSERTs, then call the safe version).
>
> Of course, you would want to wrap all this in some macros to hide the
> messy bits.
>
>> One of the routines I was dressing up today had some constraints
>> placed on a pointer passed to it. In particular, requiring the
>> pointer to reference a valid memory location in a certain
>> range (defined by manifest constants).
>>
>> ONCE THROUGH THE INVARIANTS, "live code" would further qualify
>> the parameters passed to the routine. In this case, enforcing
>> some "arbitrary" alignment criteria. Conceivably, the aligned
>> pointer could now reference something *outside* the range of
>> memory locations enforced by the invariants. The code clearly
>> has to test for this -- since the whole point is to ensure
>> those "out of bounds" areas are not referenced at run-time.
>>
>> Now, the nit pick... *technically*, the caller has complied
>> with the contractual requirements of the function (the contract
>> doesn't enforce alignment issues -- that is part of the role
>> of the function itself). As such, I maintain that the original
>> set of invariants are *all* that should be enforced (to
>> determine if the invocation "complies"). The *further*,
>> non-contractual alignment restriction that the function imposes
>> should be treated as a run-time error -- just like trying to
>> resolve "ptr[index]" when the location referenced extends
>> outside of the valid range.
>
> This I don't understand. Either it is an error to call the function with
> an unaligned pointer, or it is not an error. If it is an error, then it

Not an error (in this case).  Think:  "align to cache-lines", etc.

> is part of the requirements for calling the function, and should be
> checked and enforced as soon as possible. If it is not an error, then it
> is not an error. The function may use alignment information to run
> faster, or it may give you some debugging information to help improve
> the code, but it is not an error.

The *caller* didn't "make an error".  But, the actions that
the function WILL TAKE with the data provided could result
in those parameters being modified to what would *now* be
considered an error (had the caller supplied them in that
"form" originally).

Contrived example:

Caller invokes with argument that resolves to "249".
Function will ALIGN_UP to 100 byte boundaries so that
249 becomes "300".  (easier for me to deal with decimal
numbers at this time of night)

ASSERTs contractually constrain the parameter to the
range [1,299].

So, HAD THE CALLER PROVIDED THE 300 THAT IS *EFFECTIVELY*
BEING USED, this would have been a contractual violation.
(but, he *didn't*).

I am claiming that this should, instead, be a run-time
"error" condition (i.e., "return FAIL;") instead of
a contractual violation (i.e., "fall into debugger").

(assume FAIL is a legitimate response from the function)

[time for bed]

On 03.01.2011 16:36, D Yuniskis wrote:
> On 1/3/2011 4:58 AM, David Brown wrote:

>> One other thing you can use is static asserts. The only disadvantage
>> these have is cluttering the code, but it is not much worse than the
>> typical comment (i.e., you write "static_assert(sizeOfBuffer > 100);"
>> rather than "/* sizeOfBuffer should be at least 100 */".

> This is what I currently do (hence "ASSERT()" instead of "assert()").

Actually no, you don't.  A static assert is resolved at _compile_ time. 
  That's not what your following description says you're doing:

> However, they may or may not generate code in the production release
> (depending on how I define them). Sometimes they push an error to
> my "black box" and abort the function call (since the body of the
> function thereafter EXPECTS never to see input parameters that
> violate those stated "requirements"). Other times they may just
> "vanish"

An actual implementation of a static_assert these days (i.e. without 
special support by the language) looks roughly like this:

#define STATIC_ASSERT(condition) do{int a[1-2*(!(condition))];}while(0)

The trick is that if the condition doesn't hold, this will cause a 
compile-time error because it defines an array of negative length.  The 
problem with it are:

1) if the conditions holds, this creates an unused variable (which the 
optimizer will hopefully remove again)

2) if the conditions doesn't hold, the error message you'll get from the 
compiler has no apparent connection to the actual problem.

On 2011-01-03, Hans-Bernhard Br??ker <HBBroeker@t-online.de> wrote:

> An actual implementation of a static_assert these days (i.e. without 
> special support by the language) looks roughly like this:
>
> #define STATIC_ASSERT(condition) do{int a[1-2*(!(condition))];}while(0)
>
> The trick is that if the condition doesn't hold, this will cause a 
> compile-time error because it defines an array of negative length.  The 
> problem with it are:
>
> 1) if the conditions holds, this creates an unused variable (which
>    the optimizer will hopefully remove again)

Use a typedef instead of an actual variable definition.

> 2) if the conditions doesn't hold, the error message you'll get from
>    the compiler has no apparent connection to the actual problem.

Here's what I use:

#define __CAssert(expr,line) typedef int __compile_time_asserter_line ## line [((expr) != 0)*2 - 1]
#define _CAssert(e,l) __CAssert(e,l)
#define CAssert(expr) _CAssert(expr,__LINE__)

I don't know about other compilers, but gcc's output will include the
name of the typedef (IOW __compile_time_asserter_lineXX) in the error
message.

You'll need to ad the do{}while(0) wrapper if you want to use it where
declarations aren't allowed.  If you want to get facier, you can
probably "stringize" the expression parameter and coax the compiler to
spit it out somehow in the error message...

-- 
Grant Edwards               grant.b.edwards        Yow! Psychoanalysis??
                                  at               I thought this was a nude
                              gmail.com            rap session!!!