[Design] AZ_RESULT and simple output values. about azure-sdk-for-c HOT 14 CLOSED

@gilbertw, @antkmsft, @vhvb1989 which one do you prefer for simple functions (without output parameters)?

from azure-sdk-for-c.

I vote for option 2.

from azure-sdk-for-c.

option 2 looks simpler to maintain.
I would probably get rid of AZ_CONTRACT_ARG_NOT_NULL for now.

from azure-sdk-for-c.

@vhvb1989 yeah, we don't have AZ_CONTRACT_ARG_NOT_NULL in Azure.Core yet. I will have a PR for that. It's a macro that may return az_result.

from azure-sdk-for-c.

@JeffreyRichter, @barcharcraz ?

from azure-sdk-for-c.

I still think you need to write it with the out parameters, if we want a fancy functional interface we should deliver that as a wrapper in a functional language.

Also, this lacks a third option which is "az_result can have non-error values but is never used as a channel for the return value"

from azure-sdk-for-c.

@barcharcraz I think, we've already discussed that we may have functions that never ever return errors and in this case we would like to return a value instead of az_result. Really, they are very usefull and simplify logic and remove useless checks (like non-null output parameters). Without them, our C development life will be even harder :-)

About the third option, we may say it's the second option but with slightly adjusted meaning of what do we call error or non-error. We can still reserve the space for "non-error" but I personally would like to avoid API that uses such "non-error" things.

from azure-sdk-for-c.

This is the best version:

az_result az_span_get(az_span span, size_t i, uint8_t * out) {
  AZ_CONTRACT_ARG_NOT_NULL(out);

  if (span.size <= i) {
    return AZ_ERROR_OUT_OF_RANGE;
  }
  *out = span.begin[i];
  return AZ_OK;
}

If we do contract checking, then we should check that i is >=0 && i < span.size and return an az_result for failing this contract. i being out of range is also a contract violation - not an error.

These code samples are a good step but really, when we compare approaches we need to see what the calling/customer code would look like; not what our code would look like.

from azure-sdk-for-c.

@barcharcraz I've added an option 3. One of the problems with this option, it is more error prone because it's not obvious from a function signature that there is a special non-error case AZ_NO_MORE_ITEMS that requires special attention.

from azure-sdk-for-c.

@JeffreyRichter I agree, usage examples would be good.

az_result az_span_get(az_span span, size_t i, uint8_t * out) {
  AZ_CONTRACT_ARG_NOT_NULL(out);
  // we can't use `span.size - 1` because it may overflow in case of `span.size == 0` because `size_t` is unsigned integer.
  AZ_CONTRACT_ARG_IN_RANGE(i, 0, span.size);
  // or AZ_CONTRACT_ARG_ARRAY_INDEX(i, span.size);

  *out = span.begin[i];
  return AZ_OK;
}

from azure-sdk-for-c.

@antkmsft, @vhvb1989, @gilbertw I've added an option # 3. When az_result may have non-error/non-value status.

I still vote for # 2.

from azure-sdk-for-c.

By the way: AZ_CONTRACT_ARG_... requires that the function returns az_result. Example of AZ_CONTRACT_ARG_NOT_NULL:

#define AZ_CONTRACT_ARG_NOT_NULL(arg) \
  do { \
    if ((arg) == NULL) { \
      return AZ_ERROR_ARG; \
    } \
  } while (0)

from azure-sdk-for-c.

@katmsft, @rafilho, @mamokarz

from azure-sdk-for-c.

We've settled on the az_result return with out parameters.

az_result az_span_get(az_span span, size_t i, uint8_t * out) {
  AZ_CONTRACT_ARG_NOT_NULL(out);
  // we can't use `span.size - 1` because it may overflow in case of `span.size == 0` because `size_t` is unsigned integer.
  AZ_CONTRACT_ARG_IN_RANGE(i, 0, span.size);
  // or AZ_CONTRACT_ARG_ARRAY_INDEX(i, span.size);

  *out = span.begin[i];
  return AZ_OK;
}

from azure-sdk-for-c.