Solve #1139 by returning optionals where C-API returns NULL for objects

[sternmull]

This should solve #1139

[ekigwana]

Thank you @sternmull, I'll continue our evaluation of this interface.

As an aside, we need to install iiopp.h when bindings are enabled https://github.com/analogdevicesinc/libiio/blob/7ae483696affb64ab6e1766f55e47f00456593d0/CMakeLists.txt#L670

[sternmull]

Somehow I missed the Context when looking for the functions that needed to be changed. Updated the code.

[ekigwana]

I have updated my test program (Note that we are dealing with an ADRV9009-ZU11EG for our project and it has over 1000 attributes)

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

/*
 * hwmon3: axi_fan_control
 *         3 channels found:
 *                 fan1:  (input)
 *                 3 channel-specific attributes found:
 *                         attr  0: fault (fan1_fault) value: 0
 *                         attr  1: input (fan1_input) value: 1906
 *                         attr  2: label (fan1_label) value: FAN
 *                 temp1:  (input)
 *                 2 channel-specific attributes found:
 *                         attr  0: input (temp1_input) value: 49642
 *                         attr  1: label (temp1_label) value: SYSMON4
 *                 pwm1:  (input)
 *                 1 channel-specific attributes found:
 *                         attr  0: pwm1 value: 64
 *         No trigger on this device
 */

int main(int argc, char ** argv)
{
    try
    {
        auto iio_ctx = iiopp::create_context(nullptr, nullptr);
        {
                auto dev = iio_ctx->find_device("aaxi_fan_control");
                assert(!dev);
        }

        auto dev_name = "axi_fan_control";
        auto dev = iio_ctx->find_device(dev_name);
        assert(dev);

        {
                auto attr = dev->find_attr("non-existent");
                assert(!attr);
                auto ch1 = dev->find_channel("non-existent", false);
                assert(!ch1);
                auto ch2 = dev->find_channel("fan1", true);
                assert(!ch2);
        }

        auto ch_id = "fan1";
        auto ch = dev->find_channel(ch_id, false);
        assert(ch);

        {
                auto attr = ch->find_attr("non-existent");
                assert(!attr);
        }

        auto attr_name = "input";
        auto attr = ch->find_attr(attr_name);
        assert(attr);

        {
                auto name = dev->name();
                std::cout << "dev name: " << ((name) ? *name : "unknown") << std::endl;
                assert(name);
                assert(std::strcmp(*name, dev_name) == 0);
        }

        {
                auto id = ch->id();
                std::cout << "ch id: " << id << std::endl;
                assert(std::strcmp(id, ch_id) == 0);

                auto name = ch->name();
                std::cout << "ch name: " << ((name) ? *name : "unknown") << std::endl;
                assert(!name);
        }

        {
                auto name = attr->name();
                std::cout << "attr name: " << ((name) ? name : "unknown") << std::endl;
                assert(name);
                assert(std::strcmp(name, attr_name) == 0);
        }
    }
    catch (iiopp::error & e)
    {
        std::cerr << "ERROR " << e.code().value() << ": " << e.what() << std::endl;
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

So far that works. I am not fond of the optional deleted objects that cannot be reused in a loop. A pointer would have had the same effect and can be reassigned but i understand the design principal per the issue text.

[sternmull]

I am not fond of the optional deleted objects that cannot be reused in a loop. A pointer would have had the same effect and can be reassigned but i understand the design principal per the issue text.

I don't want to force any design decision. But I also don't see the problem. std::optional and the wrapper classes themself are very lightweight objects. I would expect any reasonable optimizing compiler to make them basically zero cost. Also I don't understand why you would not be able to reuse them the same way as you could with pointers. Maybe you can give a small example where it is inconvenient or inefficient?

A pattern that I find very useful with optionals (works for pointers too):

if (auto attr = dev->find_attr("foo")) // Makes sure attr is only visible where it is safe to dereference
{
  attr->bar(); // safe to use attr
}

But I am not sure if this is related to your critique.

[ekigwana]

I am not fond of the optional deleted objects that cannot be reused in a loop. A pointer would have had the same effect and can be reassigned but i understand the design principal per the issue text.

I don't want to force any design decision. But I also don't see the problem. std::optional and the wrapper classes themself are very lightweight objects. I would expect any reasonable optimizing compiler to make them basically zero cost. Also I don't understand why you would not be able to reuse them the same way as you could with pointers. Maybe you can give a small example where it is inconvenient or inefficient?

A pattern that I find very useful with optionals (works for pointers too):

if (auto attr = dev->find_attr("foo")) // Makes sure attr is only visible where it is safe to dereference
{
  attr->bar(); // safe to use attr
}

But I am not sure if this is related to your critique.

After further review, I do not expect it to be a problem at all please disregard that comment.

[cristina-suteu]

@mhennerich @dNechita can I merge this ?

[mhennerich]

@mhennerich @dNechita can I merge this ?

Yes