Pointer checking for NULL

[sbooth]

In the C version several functions take astro_time_t * function parameters:

• Astronomy_SiderealTime
• Astronomy_Equator
• Astronomy_ObserverVector
• Astronomy_ObserverState
• Astronomy_Horizon
• Astronomy_HourAngle
• Astronomy_RotationAxis
• Astronomy_Rotation_EQD_EQJ
• Astronomy_Rotation_EQD_ECL
• Astronomy_Rotation_EQD_ECT
• Astronomy_Rotation_EQD_HOR
• Astronomy_Rotation_EQJ_EQD
• Astronomy_Rotation_EQJ_ECT
• Astronomy_Rotation_EQJ_HOR
• Astronomy_Rotation_ECL_EQD
• Astronomy_Rotation_ECL_HOR
• Astronomy_Rotation_ECT_EQJ
• Astronomy_Rotation_ECT_EQD
• Astronomy_Rotation_HOR_EQD
• Astronomy_Rotation_HOR_EQJ
• Astronomy_Rotation_HOR_ECL

In many cases the passed parameter is dereferenced without first checking for NULL. It would be beneficial to add a check similar to:

if (time == NULL)
   return xxxErr(ASTRO_INVALID_PARAMETER);

To prevent a SIGSEGV.

[cosinekitty]

I thought about this too, but I have mixed feelings about it. It's not obvious whether the bug of passing in NULL would be easier or harder to diagnose with an error code or a crash. I could see this slowing people down figuring out what was wrong with their code. All of these calls should be passing in &time, so someone would have to go out of their way to intentionally pass NULL. I guess I could be talked into it, but I'm not completely sold yet.

[sbooth]

I've always felt that validating user parameters, particularly pointers, is something a public-facing API should do, whether it's through the hard-line path of asserts/preconditions or the gentler method of returning error codes. It makes problems easier to diagnose because the error is generated by the function you're calling as a library client and not by an internal function down the call stack that might make things more difficult to understand. As you point out for most of these functions the caller should be passing &time so in practice it likely won't be an issue but to me validating that pointers aren't NULL before dereferencing (unless other behavior is documented) is a strongly-held opinion.

[cosinekitty]

OK, that sounds reasonable. I guess I was only thinking about being a developer and diagnosing things, but I should also consider that Astronomy Engine could be part of a much larger system that isn't expecting to crash in the first place. I will fix it, and thanks for taking the time to reach out!