A get_from_variant function in C++

Question

This is the follow-up question for A recursive_transform Function For Various Type Nested Iterable With std::variant Implementation in C++. As G. Sliepen's answer mentioned, leaving only recursively transforming operation for recursive_transform() may be a better idea. As the result, the implementation of recursive_transform function is kept in the following form. Moreover, the forward declarations have been removed.

template<class T, class _Fn> requires is_iterable<T>
static inline T recursive_transform(const T input, _Fn func)
{
    T returnObject = input;

    std::transform(input.begin(), input.end(), returnObject.begin(), func);
    return returnObject;
}

template<class T, class _Fn> requires is_iterable<T> && is_element_iterable<T>
static inline T recursive_transform(const T input, _Fn func)
{
    T returnObject = input;
    std::transform(input.begin(), input.end(), returnObject.begin(),
        [func](const auto& element)
        {
            return recursive_transform(element, func);
        }
    );
    return returnObject;
}

However, I still want to handle the compound structure with ranges and std::variant, such as std::vector<std::variant<double>>. A new function get_from_variant comes up in my mind in order to focus on the operations with these things.

template<typename T_variant, typename T>
static inline auto get_from_variant(T_variant input_variant)
{
    T return_val;
    std::visit([&](auto&& arg)
        {
            return_val = static_cast<T>(arg);
            return arg;
        },
        input_variant);
    return return_val;
}

The tests of this get_from_variant function:

int main()
{
    //  get_from_variant function test
    std::variant<double> testNumber = 1;
    std::cout << get_from_variant<decltype(testNumber), double>(testNumber);
    
    //  The usage of recursive_transform function and get_from_variant function
    std::variant<double> variant_number = 3.14;
    
    std::vector<decltype(variant_number)> testVector1;
    testVector1.push_back(variant_number);
    testVector1.push_back(variant_number);
    testVector1.push_back(variant_number);
    std::cout << get_from_variant<std::variant<double>, double>(recursive_transform(testVector1, [](auto x){ return get_from_variant<std::variant<double>, double>(x) + 1; }).at(0)) << std::endl;
    
    return 0;
}

All suggestions are welcome.

• Which question it is a follow-up to?

  A recursive_transform Function For Various Type Nested Iterable With std::variant Implementation in C++

• What changes has been made in the code since last question?

  In order to handle the compound structure with ranges and std::variant, such as std::vector<std::variant<double>> in a better way, a new function get_from_variant has been created.

• Why a new review is being asked for?

  In my opinion, I am not sure whether the design of the function get_from_variant is good? Is the idea or the usage good or not? Any comment is welcome.

Answer 1

I haven't been following this thread from the beginning, so I'm more confused than you expect readers to be by this point. It would be a good idea for you to provide a complete compilable example each time — even just as a Godbolt link, if you want to keep the question's focus on some little piece of the code.

In fact, I prefer to see a Godbolt link (in addition to seeing the code in the question as you correctly have done), as it saves me the trouble of pasting your code into Godbolt myself. :) Here's a link to your code: Godbolt.

---

std::variant<double> testNumber = 1;

This doesn't compile in C++20. Did it use to? If so, yikes, that's a pretty big API break for C++... but not your problem. Anyway, change it to 1.0 and recompile.

---

template<typename T_variant, typename T>
static inline auto

Lose the static inline. Templates are effectively inline by definition, and you don't want this template to be static — you don't want to force each translation unit to keep its own unique copy (in the case that it's not optimized away by the inliner).

I'm not a fan of Giraffe_case. Template parameter names should be short and CamelCase; here I recommend V.

Your std::visit lambda has a useless return arg;. In fact, this entire function should look more like

template<class V, class T>
auto get_from_variant(V input) {
    return std::visit([&](auto&& arg) {
        return static_cast<T>(arg);
    }, input);
}

With the cruft removed, we have brain cells free to focus on the next level of pedantry: You're taking arg by forwarding reference (auto&&), but you're not actually forwarding it to the static_cast. Maybe we should use static_cast<T>(static_cast<decltype(arg)>(arg)) here, so that if arg is an rvalue reference, it'll get moved into T's constructor?

But wait; arg will never be an rvalue reference, because we're visiting an lvalue input! So maybe we shouldn't expect to modify the arg we visit — we could take it as const auto& arg. But if we don't expect to modify input, maybe it should be taken by— yeah, wait a minute, why are we making a copy of input here? Just take it by const reference to begin with!

template<class V, class T>
auto get_from_variant(const V& input) {
    return std::visit([](const auto& arg) {
        return static_cast<T>(arg);
    }, input);
}

I've dropped the [&] from the lambda, since it doesn't require any captures.

We should also look at the template parameters to get_from_variant. V can be deduced and T can't; it always always always makes sense to put non-deducible parameters first.

template<class T, class V>
auto get_from_variant(const V& input) {
    return std::visit([](const auto& arg) {
        return static_cast<T>(arg);
    }, input);
}

Now our main driver looks like this:

std::variant<double> testNumber = 1.0;
std::cout << get_from_variant<double>(testNumber);
    
std::vector testVector1 = {
    std::variant<double>(3.14),
    std::variant<double>(3.14),
    std::variant<double>(3.14),
};
std::cout << get_from_variant<double>(
    recursive_transform(testVector1, [](const auto& x){
        return get_from_variant<double>(x) + 1;
    }).at(0)
) << std::endl;

---

Meanwhile, in recursive_transform, you have a typo: const T input when you meant const T& input. You can mechanically grep for these typos, and you should!

• Again, remove static inline from templates.

• The name _Fn is reserved for the implementation; just use F.

• Copying func into the lambda isn't necessary; you should use [&] as your default for every lambda you write (unless, as above, you can get away with plain []).

• Honestly, unless you are rabid about following STL idioms, just pass the callback F by const reference and avoid ever copying it. There is a place in C++ for stateful, mutable callbacks, but transform is not that place.

• Your base case is more complicated than it needs to be. Let's fix that.

Putting it all together:

template<class T, class F>
T recursive_transform(const T& input, const F& f) {
    return f(input);
}

template<class T, class F> requires is_iterable<T>
T recursive_transform(const T& input, const F& f) {
    T returnObject = input;
    std::transform(input.begin(), input.end(), returnObject.begin(),
        [&](const auto& element) {
            return recursive_transform(element, f);
        }
    );
    return returnObject;
}

And then, it really seems to me that using std::transform here is overkill: it reads from input twice, once to make the copy and again to do the transform. Suppose we just open-coded it, like this?

template<class T, class F> requires is_iterable<T>
T recursive_transform(const T& input, const F& f) {
    T output = input;
    for (auto&& elt : output) {
        elt = recursive_transform(elt, f);
    }
    return output;
}

Of course we could use C++20 Ranges to do something like this:

template<class T, class F> requires is_iterable<T>
T recursive_transform(const T& input, const F& f) {
    auto transformed = input | std::views::transform([&](auto&& x) {
        return recursive_transform(x, f);
    });
    return T(transformed.begin(), transformed.end());
}

That's slower to compile and generates bigger code — but it might indeed be faster at runtime, if T::value_type is expensive to copy, because we're eliminating the copy-assignments on T::value_type — we're just constructing directly in place.