Discussion:
Code guidelines
(too old to reply)
Kaz Kylheku
2024-09-04 14:02:32 UTC
Permalink
The contract is
* obj->member1 CAN be null
* obj->member1->member2 CANNOT be null
* obj->member1->member2->member3 CAN be null
Newer languages have null-safe object access operators:

if (obj?->member1->member2?->member3) ...

We get the correct check, and, at a glance, the question marks annotate
what the coder believes is the contract: obj may be null,
obj->member1->member2 may be null.

GCC could easily get this extension, it seems.

obj?->member is just obj ? obj->member : nullptr except that
obj is evaluated only once.
--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @***@mstdn.ca
David Brown
2024-09-04 18:14:42 UTC
Permalink
I also have a interesting sample with linked list
Speaking generally rather than specifically about this code, your
extra checks are redundant when the code is run from a single thread,
and insufficient if it is later used from multiple threads (accessing
the same data).  But extra checks can fool people into thinking it is
safe - since the checks are unnecessary for sequential code they must
have been put there to catch rare race conditions.
There are times when you want seat-belts /and/ airbags.  But focusing
on that instead of making sure the driver is competent is not helpful!
In Cake (my front-end), I made `assert` a special built-in function to
allow it to be used by static analysis.
`assert(p != 0);`
Static analysis will then assume that `p` is not zero after this line.
It will (or at least, it can), if the assert is enabled. If NDEBUG is
defined then the standard assert macro does nothing - it does not tell
the compiler anything. And compilers - baring bugs, as always - do not
optimise by guessing assumptions.
Does assert overrides what the compiler already knows?
If you tell the compiler two contradictory things, you can't expect good
results. It is reasonable for it to believe the first thing you said,
or the second, or neither, or both at different times. Ideally, of
course, it will give a warning.

Do not lie to your compiler. It will result in tears - /your/ tears,
not the compiler's.
int i = 1;
assert(i == 2);
Here, the compiler knows `i` is `1`, but the `assert` (i.e., the
programmer) is claiming that it’s `2`!
You can't expect good results from nonsense - at best, you can hope for
a warning.

This is one reason why I use my own macro:

extern void __attribute__((error("Assume failed"))) assumeFailed(void);
#define Assume(x) \
do { \
if (__builtin_constant_p(x)) { \
if (!(x)) { \
assumeFailed(); \
} \
} \
if (!(x)) __builtin_unreachable(); \
} while (0)

(It's actually a bit more complicated since, like standard assert(), it
supports enabling or disabling. But that's the key part.)

This will catch the mistake in your example, if you use "Assume" instead
of "assert".
I then reconsidered my view. `Assert` is about what the programmer is
stating to be true. (Programmer can be wrong) It is not a "override
command".
I think this is applicable for any "assume concept".
C++ 23 added [[assume( expression )]]
"Tells the compiler to assume that an expression will always evaluate to
true at a given point ..." "The purpose of an assumption is to allow
compiler optimizations based on the information given"
"IMPORTANT: DANGEROUS OPERATION, USE WITH CARE"
https://en.cppreference.com/w/cpp/language/attributes/assume
"[[assume(E)]];" in C++23, or "__attribute__((assume(E)));" as an
extension in newer gcc in any C or C++ standard, is basically the same
as "if (!(E)) __builtin_unreachable();".

My "Assume" macro is better in most cases, because it catches errors
that are seen at compile time, while the assume attribute would
eliminate code with no warning.
I also may add this [[assume]] to cake, but assert also gives me a
runtime check.
In the first example `assert(p != 0)`, the compiler knows that `p` is
MAYBE `null`. But the programmer is asserting that `p` cannot be `null`,
so the compiler will follow the programmer's assertion.
Perhaps the programmer knows something the compiler doesn’t?
If you want the best from your compiler (optimisations and error
checking), tell it what you know.
This is similar to a case with a linked list, where the programmer knew
that if the `head` is `null`, then the `tail` must also be `null`.
However, if the compiler is certain about a fact and encounters an
`assert` with conflicting information, it should trigger a warning.
An "assert" macro could be defined to do that, if you are happy to use
the gcc extension "__builtin_constant_p()". If not, then I don't think
it is possible to specify that it should trigger a warning in any clear
way unless the expression is a constant expression - and then you'd be
using static_assert().
int i = 0;
assert(i != 0); // Warning
So make a better "assert" that does what you want.
When the compiler finds an assert, that is not adding extra information
but only saying what the compiler already knows, then we don't have
warnings. But, this is a redundant check.
int i = 0;
assert(i == 0); // ok
On the other hand, redundant 'if' I have a warning.
if (i == 0) {} //warning 'i' is always 0 here!
So, basically.
- if assert has a conflicting information compared with what compiler
already knows, then it is a warning. Compiler will not override what it
knows?
- If the assert gives the exactly information compiler already have,
then is is not a warning. Both compiler and programmers knows the same
thing.
- If the compiler is not sure about a fact and the programmer gives a
plausible fact then the compiler will assume the programmer is correct.
David Brown
2024-09-04 18:20:55 UTC
Permalink
Post by Kaz Kylheku
The contract is
* obj->member1 CAN be null
* obj->member1->member2 CANNOT be null
* obj->member1->member2->member3 CAN be null
if (obj?->member1->member2?->member3) ...
We get the correct check, and, at a glance, the question marks annotate
what the coder believes is the contract: obj may be null,
obj->member1->member2 may be null.
GCC could easily get this extension, it seems.
obj?->member is just obj ? obj->member : nullptr except that
obj is evaluated only once.
gcc is not as keen on making new extension syntax as it was some 30 year
ago. They try to keep extensions to a minimum, or fitting them into
existing patterns (__builtin functions and __attribute__'s). Or they
import new features from later C or C++ standards back to older standard
versions.

And they tend to have the attitude that programmers who want a language
that does more than current C, will probably use a different language.
In C++ you can often specify non-null requirements using references
rather than pointers, and it's not hard to make a little class template
that act mostly like a pointer except that it checks for null first and
throws an exception on null. (Making something that will chain like
your code, resulting in a bool true if everything is good to go, is I
believe possible but a bit more fiddly. Ask for details down the hall.)
David Brown
2024-09-03 14:49:48 UTC
Permalink
I am of the opinion that if something is clearly specified, you make
sure it is true when you are responsible for it - and then you can
assume it is true when you use the results.  It makes no sense to me
to do something, and then immediately check that it is done.
Whenever possible, these specifications should be in code, not
comments - using whatever tools and extra features you have
available for use. Macros and conditional compilation help make code
more portable, and can also let you turn compile-time assumptions
into run-time checks during debugging.
Yes, this contract "function don't returns null" is very
straightforward for the caller and for the implementer.
The contract implementation can be checked inside function
implementation. (One place to check the contract implementation)
The contract usage, is checked in each caller, but very straightforward.
On the other hand, struct members have much more places where the
contract can be broken. Each function that have a non const access to
user->name could break the contract implementation.
The risk is much bigger compared with the return case.
You are asking about "guidelines".  Yes, it is possible for code to
break specifications.  The guideline is "don't do that".
The guidelines are more about the usage of runtime checks, are they
required? optional? etc.. when to use etc...
The guideline is "follow the specifications". Then it is obvious that
run-time checks are not required unless you are dealing with code that
does not follow the specifications.

Now, as you note below, code under development might not follow the
specifications - that is why it is useful to do this sort of thing with
macros that support adding run-time checks during development or
debugging. Standard C "assert" is a poor man's version of this.
If you are mixing untrusted code with trusted code, then you have to
check the incoming data just like you do with data from external
sources.  But most functions are not at such boundaries.
yes ..agree.
So I think it may make sense to have redundant runtime checks, but it
must be clear the the "compile time contract" is not that one.
The first sample my create confusion (is name optional?)
void f(struct user* user)
{
      if (user->name && strcmp(user->name, "john") == 0)
      {
         //...
      }
  }
void f(struct user* user)
{
      assert(user->name);
      if (user->name && strcmp(user->name, "john") == 0)
      {
         //...
      }
  }
would show redundancy but making clear the contract still "name
should not be null"
No, redundant runtime checks are not helpful.  If they are redundant,
they are by definition a waste of effort - either run-time efficiency,
or developer efficiency, or both.  And they are a maintenance mess as
changes have to be applied in multiple places.
But we know , when the code is new (under development) the contract may
not be so clear or change very frequently.
Then it is even worse to have redundant checks - when they are
inconsistent, you have no idea what is going on or what /should/ be
going on.
     void f(struct user* user)
     {
         if (!user->name) __builtin_unreachable();
         if (strcmp(user->name, "john") == 0) {
             //...
         }
     }
Now it is /really/ clear that user->name should not be null, and
instead of two run-time checks doing the same thing, there are no
run-time costs and the compiler may even be able to optimise more from
the knowledge that the pointer is not null.
In practice I use a macro called "Assume" here, which will give a hard
compile-time error if the compiler can that the assumption is false.
It also supports optional run-time checks depending on a #define'd
flag (normally set as a -D compiler flag), as an aid to debugging.
Write things clearly, once, in code.
If the code is boundary code, then you need a check - but then a
simple null pointer check is unlikely to be sufficient.  (What if the
pointer is not null, but not a valid pointer?  Or it does not point to
a string of a suitable length, with suitable character set, etc.?)
The macro I need is something like
"I know this check is redundant, but I will add it anyway" "this check
is redundant but according with the contract"
If a check is redundant, and the compiler can see that, the extra check
will be eliminated by optimisation. So you are wasting developer time
for nothing.
assert/assume is more
"trust me compiler, this is what happens" someone else is checking this
we don't need to check again.
That is not what "assert" is, but it is roughly what my "Assume" is.
And yes, that is what you should be aiming for in the code.
Kaz Kylheku
2024-09-03 15:10:50 UTC
Permalink
Witch style (F1, F2 ..) better represents the guidelines of your code?
//The function f never returns nullptr
int * f();
^^^^^^

Nope.
void F1() {
int* p = f();
^^^^

Nope.
if (p == nullptr) {
assert(false);
return;
}
// using *p ...
}
void F2()
{
int* p = f();
^^^

Nope.
assert(p != nullptr);
// using *p ...
}
void F3()
{
int* p = f();
^^^^

Nope.
// using *p ...
}
void F4()
{
int* p = f();
And nope.

The syntax of a declaration is, roughly, "specifier-list
comma-separated-declarators".

"int" is the specifier list, "*p" is the declarator.

Your style invites optical illusions like:

int* a, b; // two pointers?

int *a, b; // Nope: actually this.

In C, declarators are type-constructing expressions which (at least in
their basic, unadorned forms) resemble how the name will be used.

For instance when you see "int a[3], *p", you can
interpret it as saying that "the expressions a[3] and *p
shall have type int".
--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @***@mstdn.ca
Blue-Maned_Hawk
2024-09-03 21:01:36 UTC
Permalink
F2 in both cases.
--
Blue-Maned_Hawk│shortens to Hawk│/blu.mɛin.dʰak/│he/him/his/himself/Mr.
blue-maned_hawk.srht.site
Keith Thompson
2024-09-04 07:22:45 UTC
Permalink
David Brown <***@hesbynett.no> writes:
[...]
Before you put any check in code, think about the circumstances in
which it could fail. If there are no circumstances, it is redundant
and counter-productive.
[...]

One thing to consider is that if a check can never actually fail the
recovery code *cannot be tested* (and you can't get 100% code coverage).

p = NULL; // assume p is not volatile
if (p != NULL) {
do_something(); // can never execute this
}

Of course not all such cases are so easily detectible (
--
Keith Thompson (The_Other_Keith) Keith.S.Thompson+***@gmail.com
void Void(void) { Void(); } /* The recursive call of the void */
David Brown
2024-09-04 10:47:30 UTC
Permalink
Post by Keith Thompson
[...]
Before you put any check in code, think about the circumstances in
which it could fail. If there are no circumstances, it is redundant
and counter-productive.
[...]
One thing to consider is that if a check can never actually fail the
recovery code *cannot be tested* (and you can't get 100% code coverage).
p = NULL; // assume p is not volatile
if (p != NULL) {
do_something(); // can never execute this
}
Of course not all such cases are so easily detectible (
I wrote "in almost all cases, it is never tested" - but as you say, in
some cases it /cannot/ ever be tested because the test conditions can
never be triggered.

I think, however, that "could be tested, but is not tested" is worse.
I've seen cases of code that has been put in for extra checks "just to
make sure" that had not been tested, and caused more trouble.

One case I remember was some extra checks for timeouts in some
communications. The new checks were unnecessary - a higher level
timeout mechanism was already in place, tested, and working. The new
checks were never tested, and never triggered during normal operation.
But when a 32-bit millisecond counter rolled over, the check was wrong
triggered - and the handling code was buggy and hung. Thus the
unnecessary and untested extra check resulted in systems hanging every
49 days.
Thiago Adams
2024-09-03 14:37:16 UTC
Permalink
I am of the opinion that if something is clearly specified, you make
sure it is true when you are responsible for it - and then you can
assume it is true when you use the results.  It makes no sense to me
to do something, and then immediately check that it is done.
Whenever possible, these specifications should be in code, not
comments - using whatever tools and extra features you have available
for use. Macros and conditional compilation help make code more
portable, and can also let you turn compile-time assumptions into
run-time checks during debugging.
Yes, this contract "function don't returns null" is very
straightforward for the caller and for the implementer.
The contract implementation can be checked inside function
implementation. (One place to check the contract implementation)
The contract usage, is checked in each caller, but very straightforward.
On the other hand, struct members have much more places where the
contract can be broken. Each function that have a non const access to
user->name could break the contract implementation.
The risk is much bigger compared with the return case.
You are asking about "guidelines".  Yes, it is possible for code to
break specifications.  The guideline is "don't do that".
The guidelines are more about the usage of runtime checks, are they
required? optional? etc.. when to use etc...
If you are mixing untrusted code with trusted code, then you have to
check the incoming data just like you do with data from external
sources.  But most functions are not at such boundaries.
yes ..agree.
So I think it may make sense to have redundant runtime checks, but it
must be clear the the "compile time contract" is not that one.
The first sample my create confusion (is name optional?)
void f(struct user* user)
{
      if (user->name && strcmp(user->name, "john") == 0)
      {
         //...
      }
  }
void f(struct user* user)
{
      assert(user->name);
      if (user->name && strcmp(user->name, "john") == 0)
      {
         //...
      }
  }
would show redundancy but making clear the contract still "name should
not be null"
No, redundant runtime checks are not helpful.  If they are redundant,
they are by definition a waste of effort - either run-time efficiency,
or developer efficiency, or both.  And they are a maintenance mess as
changes have to be applied in multiple places.
But we know , when the code is new (under development) the contract may
not be so clear or change very frequently.
    void f(struct user* user)
    {
        if (!user->name) __builtin_unreachable();
        if (strcmp(user->name, "john") == 0) {
            //...
        }
    }
Now it is /really/ clear that user->name should not be null, and instead
of two run-time checks doing the same thing, there are no run-time costs
and the compiler may even be able to optimise more from the knowledge
that the pointer is not null.
In practice I use a macro called "Assume" here, which will give a hard
compile-time error if the compiler can that the assumption is false.  It
also supports optional run-time checks depending on a #define'd flag
(normally set as a -D compiler flag), as an aid to debugging.
Write things clearly, once, in code.
If the code is boundary code, then you need a check - but then a simple
null pointer check is unlikely to be sufficient.  (What if the pointer
is not null, but not a valid pointer?  Or it does not point to a string
of a suitable length, with suitable character set, etc.?)
The macro I need is something like

"I know this check is redundant, but I will add it anyway" "this check
is redundant but according with the contract"

assert/assume is more

"trust me compiler, this is what happens" someone else is checking this
we don't need to check again.
Lawrence D'Oliveiro
2024-09-22 02:08:08 UTC
Permalink
assert/assume is more
"trust me compiler, this is what happens" someone else is checking this
we don't need to check again.
Alphard made a clear distinction between “assert” and “assume”.
David Brown
2024-09-03 14:10:13 UTC
Permalink
I am of the opinion that if something is clearly specified, you make
sure it is true when you are responsible for it - and then you can
assume it is true when you use the results.  It makes no sense to me
to do something, and then immediately check that it is done.
Whenever possible, these specifications should be in code, not
comments - using whatever tools and extra features you have available
for use. Macros and conditional compilation help make code more
portable, and can also let you turn compile-time assumptions into
run-time checks during debugging.
Yes, this contract "function don't returns null" is very straightforward
for the caller and for the implementer.
The contract implementation can be checked inside function
implementation. (One place to check the contract implementation)
The contract usage, is checked in each caller, but very straightforward.
On the other hand, struct members have much more places where the
contract can be broken. Each function that have a non const access to
user->name could break the contract implementation.
The risk is much bigger compared with the return case.
You are asking about "guidelines". Yes, it is possible for code to
break specifications. The guideline is "don't do that".

If you are mixing untrusted code with trusted code, then you have to
check the incoming data just like you do with data from external
sources. But most functions are not at such boundaries.
So I think it may make sense to have redundant runtime checks, but it
must be clear the the "compile time contract" is not that one.
The first sample my create confusion (is name optional?)
void f(struct user* user)
{
     if (user->name && strcmp(user->name, "john") == 0)
     {
        //...
     }
 }
void f(struct user* user)
{
     assert(user->name);
     if (user->name && strcmp(user->name, "john") == 0)
     {
        //...
     }
 }
would show redundancy but making clear the contract still "name should
not be null"
No, redundant runtime checks are not helpful. If they are redundant,
they are by definition a waste of effort - either run-time efficiency,
or developer efficiency, or both. And they are a maintenance mess as
changes have to be applied in multiple places.


void f(struct user* user)
{
if (!user->name) __builtin_unreachable();
if (strcmp(user->name, "john") == 0) {
//...
}
}

Now it is /really/ clear that user->name should not be null, and instead
of two run-time checks doing the same thing, there are no run-time costs
and the compiler may even be able to optimise more from the knowledge
that the pointer is not null.

In practice I use a macro called "Assume" here, which will give a hard
compile-time error if the compiler can that the assumption is false. It
also supports optional run-time checks depending on a #define'd flag
(normally set as a -D compiler flag), as an aid to debugging.

Write things clearly, once, in code.

If the code is boundary code, then you need a check - but then a simple
null pointer check is unlikely to be sufficient. (What if the pointer
is not null, but not a valid pointer? Or it does not point to a string
of a suitable length, with suitable character set, etc.?)
David Brown
2024-09-03 14:53:43 UTC
Permalink
...
The first sample my create confusion (is name optional?)
void f(struct user* user)
{
      if (user->name && strcmp(user->name, "john") == 0)
      {
         //...
      }
  }
void f(struct user* user)
{
      assert(user->name);
      if (user->name && strcmp(user->name, "john") == 0)
      {
         //...
      }
  }
would show redundancy but making clear the contract still "name should
not be null"
Redundant code can either indicate a programmer's mental confusion
Yes.
or
serve as a way to address potential contract violations.
No.

If specification violations are realistic (from untrusted code, or code
under development), then a /single/ check looks for violations.
/Redundant/ checks are pointless at best, and (as I have explained)
often worse than useless.

Computers are not humans that might miss something on the first glance,
then see it on the second time. Do the same check twice in the code and
you will get the same answer each time - the second check gives no benefits.
I believe the objective is to ensure that runtime checks are not
questioning the contract but rather functioning as redundant safeguards.
In other words, the programmer must demonstrate that they understand the
contract and are not messing it.
A safeguards for a very low risk situation also may indicate a mental
confusion about the risks involved. For instance, assert(2 + 2 == 4);
A redundant check is, by definition, a very low risk situation.
Thiago Adams
2024-09-03 16:37:02 UTC
Permalink
On 03/09/2024 13:23, Thiago Adams wrote:
...
I will give a sample
In my code I have
if (obj->member1 &&
    obj->member1->member2 &&
    obj->member1->member2->member3)
{
}
The contract is
 * obj->member1                   CAN be null
 * obj->member1->member2          CANNOT be null
 * obj->member1->member2->member3 CAN be null
So I can write just
if (obj->member1 &&
    obj->member1->member2->member3)
{
}
but...maybe, is better to be a little redundant here?
I think I prefer to leave "obj->member1->member2 && " even if I know
it should not be null.
if (obj->member1 &&
    obj->member1->member2 &&
    obj->member1->member2->member3)
{
}
I also have a interesting sample with linked list


struct item {
char* title;
struct item* next; //can be null
};

struct list {
struct item* head; //can be null
struct item* tail; //can be null
};


void list_push_back(struct list* list, struct item* p_item)
{
if (list->head == NULL) {
list->head = p_item;
//tail also should be null
}
else {

//When the head is null tail is also null
//if head is not null then tail is not null
//So tail is not null here.. assert works as assume
assert(list->tail != nullptr);

//next can be null or not null..however for the tail node
//next IS/MUST BE null
assert(list->tail->next == nullptr);

list->tail->next = p_item;
}
list->tail = p_item;
}

assert in this case works as "assume" and "check".

For instance,

We assume "assert(list->tail != nullptr);" because we access it
directly here

list->tail->next = p_item;


We also assume "assert(list->tail->next == nullptr)" otherwise we could
have a memory leak overriding next.
Thiago Adams
2024-09-04 11:52:31 UTC
Permalink
I also have a interesting sample with linked list
Speaking generally rather than specifically about this code, your extra
checks are redundant when the code is run from a single thread, and
insufficient if it is later used from multiple threads (accessing the
same data).  But extra checks can fool people into thinking it is safe -
since the checks are unnecessary for sequential code they must have been
put there to catch rare race conditions.
There are times when you want seat-belts /and/ airbags.  But focusing on
that instead of making sure the driver is competent is not helpful!
In Cake (my front-end), I made `assert` a special built-in function to
allow it to be used by static analysis.

For instance:

`assert(p != 0);`

Static analysis will then assume that `p` is not zero after this line.

However, this raises a question:
Does assert overrides what the compiler already knows?

For example:

int i = 1;
assert(i == 2);

Here, the compiler knows `i` is `1`, but the `assert` (i.e., the
programmer) is claiming that it’s `2`!

I then reconsidered my view. `Assert` is about what the programmer is
stating to be true. (Programmer can be wrong) It is not a "override
command".

I think this is applicable for any "assume concept".

C++ 23 added [[assume( expression )]]

"Tells the compiler to assume that an expression will always evaluate to
true at a given point ..." "The purpose of an assumption is to allow
compiler optimizations based on the information given"

There is a big warning there :
"IMPORTANT: DANGEROUS OPERATION, USE WITH CARE"

https://en.cppreference.com/w/cpp/language/attributes/assume

I also may add this [[assume]] to cake, but assert also gives me a
runtime check.

In the first example `assert(p != 0)`, the compiler knows that `p` is
MAYBE `null`. But the programmer is asserting that `p` cannot be `null`,
so the compiler will follow the programmer's assertion.

Perhaps the programmer knows something the compiler doesn’t?

This is similar to a case with a linked list, where the programmer knew
that if the `head` is `null`, then the `tail` must also be `null`.

However, if the compiler is certain about a fact and encounters an
`assert` with conflicting information, it should trigger a warning.

For example:

int i = 0;
assert(i != 0); // Warning



When the compiler finds an assert, that is not adding extra information
but only saying what the compiler already knows, then we don't have
warnings. But, this is a redundant check.

int i = 0;
assert(i == 0); // ok

On the other hand, redundant 'if' I have a warning.
if (i == 0) {} //warning 'i' is always 0 here!

So, basically.
- if assert has a conflicting information compared with what compiler
already knows, then it is a warning. Compiler will not override what it
knows?

- If the assert gives the exactly information compiler already have,
then is is not a warning. Both compiler and programmers knows the same
thing.

- If the compiler is not sure about a fact and the programmer gives a
plausible fact then the compiler will assume the programmer is correct.
Thiago Adams
2024-09-04 12:06:41 UTC
Permalink
On 04/09/2024 08:52, Thiago Adams wrote:
...
Perhaps the programmer knows something the compiler doesn’t?
The idea of programmer overriding what compiler knows it not new.

For instance, when casting from 'void*' to 'struct X' the programmer is
overriding the type, that can be anything.

But when overriding for instance, 'struct X*' to 'struct Y*' then the
programming is overriding something the compiler already knows.

The same concept of conflicting information could be applied to casts.?
Vir Campestris
2024-10-01 10:41:16 UTC
Permalink
On 03/09/2024 17:23, Thiago Adams wrote:
<snip>
but...maybe, is better to be a little redundant here?
I think I prefer to leave "obj->member1->member2 && " even if I know
it should not be null.
if (obj->member1 &&
    obj->member1->member2 &&
    obj->member1->member2->member3)
{
}
I think I'd prefer to _omit_ the check that obj->member1->member2 isn't
null.

If the code is running correctly that check will never trigger, and is
redundant, and will slow things slightly.

But if the code is not running correctly it will conceal the bug by
preventing the crash.

But circumstances may vary. You probably don't want to crash an
autopilot if at all possible!

Andy
Kaz Kylheku
2024-10-01 21:39:21 UTC
Permalink
Post by Vir Campestris
<snip>
but...maybe, is better to be a little redundant here?
I think I prefer to leave "obj->member1->member2 && " even if I know
it should not be null.
if (obj->member1 &&
    obj->member1->member2 &&
    obj->member1->member2->member3)
{
}
I think I'd prefer to _omit_ the check that obj->member1->member2 isn't
null.
If the code is running correctly that check will never trigger, and is
redundant, and will slow things slightly.
How about:

#define vassert(x) (assert(x), 1)

if (obj->member1 &&
vassert(obj->member1->member2) &&
obj->member1->member2->member3)

If NDEBUG is defined, that just reduces to 1, and is optimized
away. If NDEBUG is absent, the null pointer crash is replaced
by an assertion failure.
--
TXR Programming Language: http://nongnu.org/txr
Cygnal: Cygwin Native Application Library: http://kylheku.com/cygnal
Mastodon: @***@mstdn.ca
Thiago Adams
2024-09-03 14:12:24 UTC
Permalink
On 03/09/2024 10:33, Thiago Adams wrote:
...
The first sample my create confusion (is name optional?)
void f(struct user* user)
{
     if (user->name && strcmp(user->name, "john") == 0)
     {
        //...
     }
 }
void f(struct user* user)
{
     assert(user->name);
     if (user->name && strcmp(user->name, "john") == 0)
     {
        //...
     }
 }
would show redundancy but making clear the contract still "name should
not be null"
Redundant code can either indicate a programmer's mental confusion or
serve as a way to address potential contract violations.

I believe the objective is to ensure that runtime checks are not
questioning the contract but rather functioning as redundant safeguards.

In other words, the programmer must demonstrate that they understand the
contract and are not messing it.

A safeguards for a very low risk situation also may indicate a mental
confusion about the risks involved. For instance, assert(2 + 2 == 4);
Thiago Adams
2024-09-03 13:33:06 UTC
Permalink
Witch style (F1, F2 ..) better represents the guidelines of your code?
//The function f never returns nullptr
int * f();
void F1() {
    int* p = f();
    if (p == nullptr) {
       assert(false);
       return;
    }
    // using *p ...
}
void F2()
{
    int* p = f();
    assert(p != nullptr);
    // using *p ...
}
void F3()
{
    int* p = f();
    // using *p ...
}
void F4()
{
    int* p = f();
    if (p) {
     // using *p ...
    }
}
int * f() __attribute__((returns_nonnull));
Then F3 is appropriate.
(I would also have "(void)" in the declaration, unless I was using C23.)
The same question but for struct members
struct user {
   //Name is required, cannot be null.
   char * name;
};
void F1(struct user* user) {
    if (user->name == nullptr) {
       assert(false);
       return;
    }
    // using user->name ...
}
void F2(struct user* user) {
    assert(user->name != nullptr);
    // using user->name ...
}
void F3(struct user* user) {
    // using user->name ...
}
void F4(struct user* user) {
    if (user->name) {
      // using user->name ...
    }
}
void F5(struct user * user) {
    if (!user->name) __builtin_unreachable();
    // using user->name;
}
I am of the opinion that if something is clearly specified, you make
sure it is true when you are responsible for it - and then you can
assume it is true when you use the results.  It makes no sense to me to
do something, and then immediately check that it is done.
Whenever possible, these specifications should be in code, not comments
- using whatever tools and extra features you have available for use.
Macros and conditional compilation help make code more portable, and can
also let you turn compile-time assumptions into run-time checks during
debugging.
Yes, this contract "function don't returns null" is very straightforward
for the caller and for the implementer.

The contract implementation can be checked inside function
implementation. (One place to check the contract implementation)
The contract usage, is checked in each caller, but very straightforward.

On the other hand, struct members have much more places where the
contract can be broken. Each function that have a non const access to
user->name could break the contract implementation.
The risk is much bigger compared with the return case.

So I think it may make sense to have redundant runtime checks, but it
must be clear the the "compile time contract" is not that one.


For instance:

The first sample my create confusion (is name optional?)

void f(struct user* user)
{
if (user->name && strcmp(user->name, "john") == 0)
{
//...
}
}

But :
void f(struct user* user)
{
assert(user->name);
if (user->name && strcmp(user->name, "john") == 0)
{
//...
}
}

would show redundancy but making clear the contract still "name should
not be null"
Loading...