1

I have an API written in C, which produces a result by returning a pointer to allocated memory.
For using it with C++ (C++11) I've wrapped the function calls in objects, which keep the result in a std::shared_ptr. So far so good.

However, the C library features two functions for every operation. One produces possibly an error, the other never. Let's call them
some_pod * do_it_with_error(Parameter ..., Error **)
and
some_pod * do_it_without_error(Parameter ...)

I can pass in the address of an Error * pointer to the first function and if there's an error it won't be NULL afterwards.

For solving this I thought of two different implementations.
First, I could SFINAE for choosing between the with_error and without_error functions, like so:

template<typename NoThrow = void>
struct do_it {
  public:
    void operator()(...)
    {
      Error * error = NULL;
      m_result = std::shared_ptr<some_pod>(do_it_with_error(..., &error));
      if (error) {
        throw(MyExceptionWithError(error));
      }
    }
  private:
    std::shared_ptr<some_pod> m_result;
};

template<>
class do_it<std::nothrow_t> {
  public:
    void operator()(...) noexcept
    {
      if (! m_result) {
        m_result = std::shared_ptr<some_pod>(do_it_without_error(...));
      }
    }
  private:
    std::shared_ptr<some_pod> m_result;
};

Usage:

do_it<> di_error;
try {
  di_error(...); // might throw
} catch (...) {}

do_it<std::nothrow_t> di_no_error;
di_no_error(...); // won't throw for sure

However, since the result is wrapped in a std::shared_ptr there will also be a get() method:

const std::shared_ptr<some_pod> & get(void) { return m_result; }

For error checking I could just implement a another method check.
The default implementation would now look like this:

struct do_it {
  public:
    // will never throw
    const std::shared_ptr<some_pod> &
    get(void) noexcept
    {
      if (! m_result) {
        m_result = std::shared_ptr<some_pod>(do_it_without_error(...));
      }
      return m_result;
    }

    // might throw
    do_it &
    check(void)
    {
      if (! m_result) {
        Error * error = NULL;
        m_result = std::shared_ptr<some_pod>(do_it_with_error(..., &error));
        if (error) {
          throw ...;
        }
      }
      return *this;
    }

  private:
    std::shared_ptr<some_pod> m_result;
};

Usage:

do_it di_error;
try {
  auto result = di_error.check().get();
} catch (...) {}
do_it di_no_error;
di_no_error.get();

So, both implementations seem to be equally good (or bad) to me.
How could I decide which one to use.
What are the Pros and Cons?

  • Careful there with the shared_ptr! If it's a C API, the object it returns was allocated with malloc and therefore must be deleted with free. If you just delete is as shared_ptr does by default, it will explode on some platforms. shared_ptr can get a custom deleter, pass it one that releases using free. – Jan Hudec Feb 5 '14 at 12:14
  • 2
    What does the variant without error overload do if the error happens? – Jan Hudec Feb 5 '14 at 12:16
  • I didn't explain this in detail in my question, but it is an important detail. I've explained here more in depth. – jrk Feb 5 '14 at 12:22
1

Too complicated - wrap them both as if either could throw an error, that the 2nd fn will never throw only means the compiler will never have to call the error handling routines (and might even optimise it out entirely) - either way your code will perform just as well thanks to C++s design.

Your code will also be much easier to understand, even with this redundant error handling (that one day might be used for other, unexpected errors)

  • Are both solutions too complicated? Do you mean to wrap them in two separate classes? Like struct do_it_with_error and do_it_without_error? – jrk Feb 5 '14 at 11:17
  • wrap them as if they were not different things, one will throw exceptions the other will not - the caller wouldn't have to know which type of API call he was calling as the wrapper class would look identical regardless of which type of underlying API it was calling. I may have made as assumption based on your question but I don't know how these functions are actually organised, how many of them there are, if they are generated, etc. But a single interface for the caller is best so he wouldn't need to call check().get() for some and get() for others. – gbjbaanb Feb 5 '14 at 11:26
  • I see. The C API is generally organized in *_with_error and *_without_error. The *_without_error won't immediately produce an error, but queue it up in the event queue which belongs to the library. So, for the user it is important to know if he's calling an API function which generates an error immediately or deferred. I think that also the reason why I cannot easily wrap those functions into one method. A single interface would of course be the best solution, but I can't think of a way how to do that here. – jrk Feb 5 '14 at 11:38
  • 1
    Then I'd create 2 differently named wrappers - do_it_now and do_it_deferred (or whatever) so its really obvious, or if the user could call one or the other (ie he chooses what type of handling he wants) pass in a handle to the deferral queue reader object (or suchlike) to distinguish between the wrapper objects. Currently the 2nd option means he has to know there's a check() method to call, and he might forget or otherwise not bother calling it. That'd be bad. – gbjbaanb Feb 5 '14 at 11:53
  • Ok, I see your point here. Providing two different wrappers would mirror the C API. The check() method would obfuscate things a bit more. What about the first solution using SFINAE? That'd be just like two different method calls, but by using a template parameter. – jrk Feb 5 '14 at 12:28
1

In C the error objects are somewhat pain to handle, so there is the other overload that stores the errors somewhere. But in C++ exceptions make it all much easier to handle. So I would only wrap the variant that reports errors and converted the errors to exceptions and ignore that the other API even exists.

  • Sorry, I missed out a bit of information. I've added it above. Unfortunately I cannot just leave out a part of the functionality. – jrk Feb 5 '14 at 12:29
  • @jrk: Then simple separate wrappers are probably the best way to go. The user needs to know how to handle the errors and has to not get it wrong by mistake. – Jan Hudec Feb 5 '14 at 12:37
  • Thanks for your answer. I'd accept it too, if I could accept two answers. :) You and gbjbaanb brought me on the right track! – jrk Feb 5 '14 at 19:12
0

For each operation, you have three distinct subjects to take care about, say, workload, result and error handling. Let's see them all in isolation:

  1. Workload: shouldn't be duplicated. Always call the C function possibly producing error information, as it's the most complete of each pair.

  2. Result: PODs can be kept as they are in the C API and hold/retrieved through shared_ptr objects, but this is only one of many possible options -if, certainly, a quite straightforward one. Maybe some of this PODs are worth to be wrapped into, or converted to, copy-constructible/move-constructible classes and directly returned from the operator() call.

  3. Error handling: the way your C++ function objects do (or do not) communicate error-related information is seconday and orthogonal to the main workload. Ideally, you want to be able to change that error handling mechanism depending on the context, without affecting the workload itself. So view it as a dependency that you inject on the function object at your will, either at construction time or at calling time. This way, you can add additional error handling mechanisms in the future, other than current do-nothing and exception-throwing ones (what if you do end wanting to notify errors thorugh console messages, for example?).

I'll end providing a simple implementation that could serve as example:

class DoSomeStuff
{
public:
    class Result
    {
    public:
        Result() : _result() {}
        Result(const Result &rhs) : _result(rhs) {}
       ~Result() {}

        Result &operator=(const Result &rhs)
        {
            if(&rhs != this)
                _result = rhs._result;

            return *this;
        }

        int GetSomeData() const { return _result->_some_int_value; }

    private:
        shared_ptr<some_pod> _result;
    };

    struct IErrorHandler
    {
        virtual void NotifyError(Error &) = 0;
    };

    Result operator()(/*params*/, IErrorHandler &errorHandler)
    {
        Error *error = { nullptr };

        shared_ptr<some_pod_type> result = { do_some_stuff_with_error(/*params*/, &error) };

        if(error != nullptr)
            errorHandler.NotifyError(*error);

        return Result(result);
    }
};

Note that Result and IErrorHandler don't have to be included inside the function class; I've done this way because they're both dependencies, but can be extracted in order to avoid duplication among several function classes.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.