// Returns negative on errors.
// Sets out_size_bits to the bits serialized.
static inline int32_t serialize_foo(const foo* in_instance, uint32_t offset, uint8_t* const out_buffer, uint32_t* out_size_bits)
Option 3
// Returns size written. Returns 0 on error although 0 is not an error (check out_status).
// Sets out_status negative on errors. Optional. Set to 0 to ignore errors; you have been warned.
static inline uint32_t serialize_foo(const foo* in_instance, uint32_t offset, uint8_t* const out_buffer, int32_t* out_status)
Not an Option
// Returns negative on errors. Positive values are the size_bits written.
// This makes the return value an implicit union of int32_t and uint31_t which should tell you why this isn't an option.
static inline int32_t serialize_foo(const foo* in_instance, uint32_t offset, uint8_t* const out_buffer)
Can you please expand a bit why Not an Option is not an option?
This makes the return value an implicit union of int32_t and uint31_t which should tell you why this isn’t an option.
Are you concerned about the range of the return value being limited by the sign bit? Or are you concerned about the type safety? Type-wise this is certainly not great but the expressivity of C is limited so there’s only so much one can do.
Also, is my understanding correct that we only have two error states:
The destination buffer is too small.
The structure breaks DSDL contracts: an array is too large or a union tag is wrong.
int main()
{
ResultType result = doit();
if (result.is_error)
{
if (result.value.error == BAD_ERROR)
{
// wow, that was bad!
return -1;
}
else
{
// Some other error
return -1;
}
}
else
{
// no error
return result.value.size;
}
}
this is the same logic if the result is a signed int:
int main()
{
int32_t result = doit();
if (result < 0)
{
if (result == BAD_ERROR)
{
// wow, that was bad!
return -1;
}
else
{
// Some other error
return -1;
}
}
else
{
// no error
return (uint32_t)result;
}
}
ergo, the “not an option” option describes a tagged union albeit one that is very instruction efficient but you also have the oddity that the uint32_t is actually a uint31_t and must be sure not to use that last bit for size information. This the compiler can’t help you with. I’d rather use more instructions and another register and avoid all the edge cases that the implicit tagged union raises.
OK, I chose Option 2 because it is more in-line with what one coding in C would expect, I think.
Do we need to distinguish between the two (did I miss anything?) error states, or can we simply reduce the error state to a bool? I don’t expect an application to choose a specific error handling strategy based on the error code.
Are we also using a bool error state for deserialization?
Okay, For Option 2 I think the out variable is optional (if you don’t care about the bits read or written you can ignore…at your own peril).
I’d rather keep the error condition as a signed machine word. This means we get success checks in the implicit bool conversion and we have a more future-proof API if we did need to surface more discrete error conditions that the application might have to handle. Given the nature of our target architectures there will be no instruction or memory advantage to the BOOL.
I very much dislike 3, maybe it’s just flashbacks from using errno, but I firmly believe that error conditions should be encoded as the return value, not a seperate check.
I prefer 2 to 1 mostly because that’s the pattern I typically see/use in C, and we don’t have the advantage of a strong type system here that would make a new type very useful (a la Rust’s Result type).
And yeah, there are two categories of errors but there are 4 error values at the moment, and it may be useful to distinguish those in some applications.
Modified proposal for the serialization support header:
#define NUNAVUT_SUCCESS 0
/// Nunavut returns 0 for success and > 0 for any failure. It is always adequate to
/// check that error_value != 0 to detect errors.
///
/// Nunavut serialization will never define more than 127 errors and the reserved
/// error numbers are [1,127].
///
I don’t want to be negativ here (especially since I’m not contributing anything to the code generation PR) but I really can’t bring myself to like “POSIX-like” return values as defined in @scottdixon 's post above. Defining 0 as SUCCESS means you can’t use the implicit to bool-conversion. Using the ! negation operator in the “usual way” leads to a semantically wrong result.
if (!pthread_mutex_lock(&muc))
{
/* This is the success case :( */
}
Although I don’t like implicit conversion myself and generally try to avoid it, but the statement
if (0 == pthread_mutex_lock(&mux))
looks just horrible. Personally, I’d go for a signed return value with negative values encoding an error:
/// Nunavut serialization does not define positive error values but may do so in the
/// future. Reserved positive error values are [1,127].
If we keep the “may do so in the future” then <0 would be impossible to use. Also, I mentioned this earlier but consider this: libcanard will never be able to use positive error codes because positive values represent correct results, so using positive errors in Nunavut will be a departure from that convention. I suspect that it might be better to remove the statement about the possibility of positive error codes appearing in the future.