analogdevicesinc / libiio

A cross platform library for interfacing with local and remote Linux IIO devices
http://analogdevicesinc.github.io/libiio/
GNU Lesser General Public License v2.1
471 stars 309 forks source link

c++: Replace all asserts with throws #1139

Closed ekigwana closed 4 months ago

ekigwana commented 5 months ago

https://github.com/analogdevicesinc/libiio/blob/7ae483696affb64ab6e1766f55e47f00456593d0/bindings/cpp/iiopp.h#L256

The asserts as utilized especially in the example above result in the program terminating. This is reasonable for test program accessing a particular device but when the code is part of a larger system this is highly undesirable. There are a couple ways of dealing with this:

  1. Define NDEBUG prior to including iiopp.h
  2. Replace asserts with a MACRO that a definition can convert to NO OP
  3. Replace asserts with throws since the happy path should not incur the penalties of error checking

As an example of this undesirable behavior, if there is a hardware communication problem with a particular IIO device, it will not get enumerated by the kernel driver. Proceeding to then use the context to find the device by name then results in a terminal condition:

#include <iiopp.h>
#include <iostream>

int main(int argc, char ** argv)
{
    try
    {
        auto iio_ctx = iiopp::create_context(nullptr, nullptr);
        auto dev = iio_ctx->find_device("aaxi_fan_control"); // This results in an assert. Note misspelled name to simulate error
    }
    catch (iiopp::error & e)
    {
        std::cerr << "ERROR " << e.code().value() << ": " << e.what() << std::endl;
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

With above program producing the following output:

test: /usr/include/iio/iiopp.h:256: iiopp::impl::AttrSeqT<obj_T, get_attrs_count_T, get_attr_T, find_attr_T>::AttrSeqT(const obj_T*) [with obj_T = iio_device; unsigned int (* get_attrs_count_T)(const obj_T*) = iio_device_get_attrs_count; const iio_attr* (* get_attr_T)(const obj_T*, unsigned int) = iio_device_get_attr; const iio_attr* (* find_attr_T)(const obj_T*, const char*) = iio_device_find_attr]: Assertion `obj' failed.
Aborted

Please share your thoughts on this so I can work on a patch to resolve this issue. Also, is there a code format spec for libiio? Like a clang-format or something? I have noticed different styles within iiopp.h.

pcercuei commented 5 months ago

@sternmull, want to chime in?

sternmull commented 5 months ago

The idea is that the wrapper classes always refer to non-null pointers of the C-API object, just like C++ references would do. This is the general assumption that is checked by the assert.

The problem is that I missed that some functions might not be able to return an object under all conditions. In that situations C-API returns NULL.

My suggestion to fix this: I would go through all functions that return objects and read their documentation more carefully. If they can return NULL for an object then I change their return type to std::optional<ObjectType>.

If there is mixed style within iiopp.h then it was by accident. I did not came across a style guide or autoformatting settings for libiio. But I have to admit that I also was not looking actively for it.

ekigwana commented 5 months ago

Thanks you @sternmull