refactor: add gsl::not_null to get compile time / run time pointer guarantees

[PastaPastaPasta]

Issue being fixed or feature implemented

Current implementation relies either on asserts or sometimes checks then returning a special value; In the case of asserts (or no assert where we use the value without checks) it'd be better to make it explicit to function caller that the ptr must be not_null; otherwise gsl::not_null will call terminate.

See https://github.com/microsoft/GSL/blob/main/docs/headers.md#user-content-H-pointers-not_null and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-nullptr

I'm interested in a conceptual review; specifically on if this is beneficial over just converting these ptrs to be a reference?

What was done?

Partial implementation on using gsl::not_null in dash code

How Has This Been Tested?

Building

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[knst]

otherwise gsl::not_null will call terminate.

I think assert is better than std::terminate, because with assert you can see backtrace when app is running in debugger.

[PastaPastaPasta]

otherwise gsl::not_null will call terminate.

I think assert is better than std::terminate, because with assert you can see backtrace when app is running in debugger.

Well, we could change this to just be assert(false) then instead of calling std::terminate

[knst]

Well, we could change this to just be assert(false) then instead of calling std::terminate

Will we have in this case any error message in console (without debugger) such as "pointer X at file.cpp:NNN is null" also same as with assert? If so it's fine then

[PastaPastaPasta]

Please take a look at this further set of changes and LMK what you think? it uses an early implementation of c++20's std::source_location that I think should be low cost; provides output now like
ERROR: error detected null not_null detected at dsnotificationinterface.cpp:41:57:AcceptedBlockHeader

[knst]

Concept ACK if it works as described

Btw, looks like magic for location of call

[knst]

Std::cerr I think should be, also std::endl to flush buffer

[knst]

I think assert is better than std::terminate, because with assert you can see backtrace when app is running in debugger.

As I checked, at least gdb catches std::terminate without issues:

$ gdb -q main
Reading symbols from main...
(gdb) r
Starting program: /tmp/main 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
terminate called without an active exception

Program received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
44      ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff783c406 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff782287c in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff7ca4ee6 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007ffff7cb6e9c in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#7  0x00007ffff7cb6f07 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6
#8  0x0000555555555156 in main () at main.cpp:4
(gdb) quit
A debugging session is active.

main.cpp:

#include <exception>

int main() {
    std::terminate();
}

So, std::terminate(), abort() and assert() behave are very similar in gdb and std::terminate seems completely fine.

Probably I got confused by exit() that is used in libjpeg/jpegturbo: https://github.com/winlibs/libjpeg/blob/master/jerror.c#L69
exit() is not caught by gdb even if code is not 0 indeed.

[github-actions]

This pull request has conflicts, please rebase.

[UdjinM6]

let's maybe have both cout and LogPrintf? UdjinM6@4eb326c

[knst]

It should not be stdout, but stderr

[UdjinM6]

ACK

[PastaPastaPasta]

Would it be better to use std::endl instead to ensure that it actually flushes?

[UdjinM6]

Yeah, we could do 3d69147. I doubt it would make any difference but 🤷‍♂️

[UdjinM6]

re-ACK

[PastaPastaPasta]

LGTM