grackle-project / grackle

The Grackle chemistry and cooling library for astrophysical simulations and models.
Other
26 stars 50 forks source link

Introduce GR_SUCCESS and GR_FAIL #217

Closed mabruzzo closed 3 months ago

mabruzzo commented 3 months ago

These are macros in the public grackle.h header file. The idea is that external applications will be able to decode the error-codes in a self-consistent manner with the rest of Grackle.

In practice, the main benefit here is that the python bindings no longer need to include a private Grackle header. This will ultimately lead to a minor simplification in GH #208.

Ideally, we would go through and replace all occurences of SUCCESS with GR_SUCCESS and FAIL with GR_FAIL (so that we can entirely remove SUCCESS and FAIL from the codebase), but that would be a fairly invasive change that would touch a lot of code.

gregbryan commented 3 months ago

This seems sensible to me, although is there an argument for using the (more standard) EXIT_FAILURE and EXIT_SUCCESS macros, which I think are guaranteed to be defined?

mabruzzo commented 3 months ago

This seems sensible to me, although is there an argument for using the (more standard) EXIT_FAILURE and EXIT_SUCCESS macros, which I think are guaranteed to be defined?

So the EXIT_SUCCESS and EXIT_FAILURE macros are defined in the header. Importantly, these are intended to be used as exit-codes when the program exits (i.e. the value returned by main or the value passed to a call to exit). The C standard is agnostic about their precise values (the values are implementation defined). But according to the posix standard, EXIT_SUCCESS must be 0.

This is fundamentally at odds with the current implementation (currently the SUCCESS macro and the new, compatible GR_SUCCESS macro is defined as 1). Since most codes with error-checking probably hard-code the return-values to 0 and 1, I don't think we should rely upon EXIT_SUCCESS and EXIT_FAILURE.

A standard thing in C libraries is to define different error return codes to signify different errors. I'm not necessarily advocating for that (in fact I think there may be a better alternative), but I think the solution in this PR is more compatible with that general approach.

brittonsmith commented 3 months ago

This looks good to me. I've thought off and on about whether we should do something about Grackle essentially defining the opposite SUCCESS/FAIL values as what seems to be standard. This is a nice way to establish a standard for the code. I think I'd prefer not to propagate this until a few larger PRs are settled.