Using status for things other than status

[schwehr]

status should only be used for MB_SUCCESS or MB_FAILURE. Other uses make the code less straight forward.

e.g. from https://github.com/dwcaress/MB-System/blob/master/src/mbio/mbr_dsl120pf.c:

    int status = MB_SUCCESS;
    // SNIP
    status = fread(buffer, 1, 124, mbfp);
    if (status == 124) {
        status = MB_SUCCESS;
        *error = MB_ERROR_NO_ERROR;
    }
    else {
        status = MB_FAILURE;
        *error = MB_ERROR_EOF;
    }

fread returns size_t and the man page for fread says:

RETURN VALUES
     The functions fread() and fwrite() advance the file position indicator
     for the stream by the number of bytes read or written.  They return the
     number of objects read or written.  If an error occurs, or the end-of-
     file is reached, the return value is a short object count (or zero).

     The function fread() does not distinguish between end-of-file and error;
     callers must use feof(3) and ferror(3) to determine which occurred.  The
     function fwrite() returns a value less than nitems only if a write error
     has occurred.

It's possible that the error wasn't an EOF. Ignoring that, this code should be something like:

    const size_t bytes_expected = 124;
    const size_t num_read = fread(buffer, 1, bytes_expected, mbfp);
    const int status = num_read == bytes_expected ? MB_SUCCESS : MB_FAILURE;
    *error = status ? MB_ERROR_NO_ERROR : MB_ERROR_EOF;
    /* TODO(schwehr): Check for error conditions other than EOF. */

It would be nice if mb status and error become enums to flush out more of these beyond these:

egrep 'status = f(read|write)' *.c | wc -l
318

[dwcaress]

Kurt,

As you've noted, my original notion was to propagate status (success or failure) and condition (error id) separately so that the usual structure is: status = mb_whatever(verbose, stuff_in, &stuff_out, &error); Here status can be MB_SUCCESS=1 or MB_FAILURE=0 and error can be error=MB_ERROR_NO_ERROR=0 or some error code that is fatal if >0 and nonfatal if <0. The error codes are defined in mbio/mb_status.h. As with anything that has been haphazardly added to over a few decades, there are too many separate error codes and there is some inconsistency with respect to the error descriptions and their usage in the code.

Given that any error other than 0 corresponds to status=MB_FAILURE, it seems that a single value would suffice. Perhaps the use of two values has been an unnecessary complexity. Do you think a different structure should be adopted? Is there a structure that would be most amenable to ultimately object orienting the mbio library?

Dave

[schwehr]

There are lots of ways to slice the problem of handling errors. My suggestion for now is to just refine the existing strategy slowly into a more stable state with two enums.

Longer term, yes, there only needs to be one error handling thing around for any instance. There are lots of patterns of readers/scanners that handle errors cleanly. MB-System can do exceptions, but I am not too familiar with those as they are banned from most of Google's C++ code. What I know the best is:

https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/stubs/status.h https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/stubs/statusor.h

StatusOr<float> result = DoBigCalculationThatCouldFail();
if (!result.ok()) {
  LOG(ERROR) << result.status() << " any other text you might want";
  return result.status();
}
const auto answer = result.ValueOrDie();  // This is your float that you work with.
// on you go with processing...

A reader class can have a Status member that keeps track of the error condition.

Often done in conjunction with this is to have builders for instances and these builders return a pointer to a valid instance or a nullptr. They use LOG to record troubles.

But I suggest holding off on this discussion for a while. Things like Qt will likely force all related code to use their error handling.