Fwd: [PATCH 15/20] chainimpl.h: Add _Assert() to _Chain_Initialize_empty()

[Gedare Bloom]

On Wed, Nov 26, 2014 at 11:29 AM, Peter Dufault <dufault at hda.com> wrote:
> These are minor nits, but I'll bring them up anyway because as I review these changes I keep thinking about them.
>
> If you have a small-codespace target that can togenerate faults on low-address-space accesses then these NULL dereferences are going to be caught in the exception handler and don't help.  These assertions are adding a negligible amount of code overhead to those small-codespace targets.
>
You can optimize the asserts out. It might be sensible to add a class
of "ASSERT_NOT_NULL" in case one wants to keep other asserts but catch
NULLs in the exception handler.

> These changes are losing information about why something is not NULL.  If you're inspecting the code and see the _Assert() you think "Why will my system go into fail-safe-shutdown in this situation?  Why is this known to be not NULL?".
>
Great point. Asserts should have comments attached to them in general.

> - Could an _Assert_not_NULL() macro be used to eliminate the negligible overhead?  It could be defined differently for targets that unmap the bottom of memory.
>
Hey look, thinking the same thing, I should finish reading before writing...

> - Could an _Assert_known_not_NULL() macro be used for pointers that are absolutely, positively known to be not-NULL but somehow they may be corrupted?  That preserves information for someone looking at the code in the future.  Instead of a check-in comment that says "This can't be NULL, it was checked before so we can assert this" the reviewer will know it is supposed to have been previously checked to be not NULL, so either someone broke the code or something corrupted memory.
>
What is the difference between the previous two variants? Would you
expect to optimize out "Assert_known" in all production code?

Great points,
Gedare

>> On Nov 25, 2014, at 18:02 , Joel Sherrill <Joel.Sherrill at oarcorp.com> wrote:
>>
>> CodeSonar flagged this as a potential NULL deference. That should never
>> occur but adding the _Assert() ensures we are checking that.
>
> Peter
> -----------------
> Peter Dufault
> HD Associates, Inc.      Software and System Engineering
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel