Great idea.

Just to add, for GCC there are some differences, nuances, will share below.

Isn't _Nullable is different from attribute((nonnull)), they're actually the opposite.
nonnull attribute means the compiler optimizer will remove all nullptr handling code, that is because nonnull attribute means this parameter will never be null!

void f(const char * str) attribute((nonnull));

void f(const char * str)
{
if(nullptr != str) // Optimizer will remove this check.
{
std::cout << str;
}
}

Someone investigated this problem and saw it removed NULL checks http://www.rkoucha.fr/tech_corner/nonnull_gcc_attribute.html

wget2 removed the nonnull attribute due to the optimizer removing checks against NULL too
https://gitlab.com/gnuwget/wget2/-/issues/200

Thanks for the reminder!

Since @kbayes's deb14dc, the API reference docs are now autogenerated, which removes my main concern for these decorations: that they will clutter the API and decrease readibility.

@liuliu, @jonnygrant,

1. Would you be okay with stripping these decorations from the API docs?
2. Any other compiler decorations you find useful? Is it just _Nullable or is there something else (ignoring Nonnull for the reasons in @jonnygrant's comment)
3. Pointers to similar libraries which you think make good use of such decorations would be appreciated.

Can I look into this @yuvaltassa ?

Are there any additional considerations beyond the one mentioned above that might be helpful for me to start?

You could use a macro, to only enable the attribute nonnull on a specific build, just to get the warnings that attribute nonnull found. I wouldn't put it on a real build that was ever run. gcc with -fanalyze -02 should give some other NULL warnings if it finds some :)

@varshneydevansh sure, go ahead and see where you get with this!

Looking forward to continuing the discussion over a PR 🙂

Thank you, @yuvaltassa, I will do this soon as I fell into a little health trouble. Once I am fit, I will create a PR. =)

So, I sat down an hour ago and being able to understand what we need to do -

1. _Nullable and _Nonnull are attributes that can be added to pointers in your code to indicate to the compiler whether a pointer can be NULL or not.
   a. _Nullable indicates that the pointer can have a NULL value.
   b. _Nonnull indicates that the pointer must not be NULL.

   Using these attributes helps to catch NULL pointer bugs at compile time rather than at runtime.

2. We need to go through the mujoco.h file and identify which pointers can be NULL and which cannot.

3. Not all compilers support the _Nullable and _Nonnull attributes. For example, the clang compiler supports these attributes, but the GCC compiler does not.

I think I need to add something like this in the file also -

#ifdef __clang__
#define _Nullable _Nullable
#define _Nonnull _Nonnull
#else
#define _Nullable
#define _Nonnull __attribute__((nonnull))
#endif

1. Yup, that's correct.
2. Yes.
3. Ya you'd need to hide these behind compiler specific define, like e.g., here.
4. Asking the thread: are there any other decorators that would be valuable to have?

https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Function-Attributes.html

gcc follows a different format. Useful when not all parameters can't be NULL. I give an example below with only 3 of the 5 parameters specified as never NULL

extern void *
my_memcpy (void *dest, const void *src, size_t len, void * other, const void * extra)
attribute((nonnull (1, 2, 5)));

Anyway, ensure to only do it on a build for warnings, as otherwise all your if(NULL != ptr) checks will be optimized out by the compiler.

So should I add something like this in the src/engine/engine_macro.h

#ifdef __clang__
#define _Nonnull _Nonnull
#define _Nullable _Nullable
#elif defined(__GNUC__)
#define _Nonnull __attribute__((nonnull))
#define _Nullable
#else
#define _Nonnull
#define _Nullable
#endif

I am also a little unclear about the reference in for the other decorators, which might be useful for us -

https://github.com/deepmind/mujoco/blob/02e819e76a505857cf79cca9ec4183489ceb8341/src/engine/engine_macro.h#L26

The above snippet reference is defining mjTHREADLOCAL as __declspec(thread) when compiled with MSVC and as _Thread_local otherwise.

Furthermore, GCC does not have an equivalent of _Nullable, and it will expand to nothing for the GCC

Does that _Nonnull compile with latest GCC? I don't recall it being supported within the parameter list.

I thought always need to specify the argument number, you can use my marco :

#define NONNULL_ARG_NUM(x, y) attribute((nonnull(x, y)))

I have added the macros in the file include/mujoco/mjmacro.h

//-------------------------- nullability check ----------------------------------------------------

#ifdef __clang__
#define NONNULL_ARG(x) _Nonnull
#define NULLABLE _Nullable
#elif defined(__GNUC__)
#define NONNULL_ARG(x) __attribute__((nonnull(x)))
#define NULLABLE
#else
#define NONNULL_ARG(x)
#define NULLABLE
#endif

and the changes to the function I am doing like this -

example -

// load model from file and check for errors
   m = mj_loadXML("hello.xml", NULL, error, 1000);

in mujoco.h -

// Parse XML file in MJCF or URDF format, compile it, return low-level model.
// If vfs is not NULL, look up files in vfs before reading from disk.
// If error is not NULL, it must have size error_sz.
MJAPI mjModel* mj_loadXML(const char* NONNULL_ARG(1) filename, const mjVFS* NULLABLE vfs, char* NULLABLE error, int error_sz);

Fine, I made some mistakes with the above approach and did modify a lot of function definitions in the mujoco.h file with the above approach.

After, a bit of more dig up, I think this is what we are looking for and now correcting my mistake-

https://chromium.googlesource.com/v8/v8/+/99d31b432f6df5a1a67bbc69ca33ef80cb67e4c3/include/v8config.h#380

define V8_NONNULL(...) attribute((nonnull(VA_ARGS)))

#ifdef __clang__
#define NONNULL_ARG _Nonnull
#define NULLABLE_ARG _Nullable
#define NONNULL_FUNC(...)                         /* NOT SUPPORTED */
#elif defined(__GNUC__)
#define NONNULL_ARG                               /* NOT SUPPORTED */
#define NULLABLE_ARG                               /* NOT SUPPORTED */
#define NONNULL_FUNC(...) __attribute__((nonnull(__VA_ARGS__)))
#else
#define NONNULL_ARG                              /* NOT SUPPORTED */
#define NULLABLE_ARG                             /* NOT SUPPORTED */
#define NONNULL_FUNC(...)                      /* NOT SUPPORTED */
#endif

// Parse XML file in MJCF or URDF format, compile it, return low-level model.
// If vfs is not NULL, look up files in vfs before reading from disk.
// If error is not NULL, it must have size error_sz.

MJAPI mjModel* mj_loadXML(const char* NONNULL_ARG filename, const mjVFS* NULLABLE_ARG vfs, char* NULLABLE_ARG error, int error_sz) NONNULL_FUNC(1);

I am a little lost with the modification for the https://mujoco.readthedocs.io/en/latest/APIreference/APIfunctions.html?highlight=mj_multiRay#ray-collisions

I am creating the PR so that I can get a better feedback. =)

varshneydevansh,

How about adding this comment, so it's clear it's for the optimizer :

/* _Nonnull allows the compiler to opimize out all the NULL pointer checks */

Can someone edit the title of this PR so it is the correct spelling of _Nonnull?