2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ get_from_variant() might not be the best name, as there's already std::get() for variants. Some languages already have a variant_cast, but that casts between two different variants. Maybe get_as() is better? \$\endgroup\$ – G. Sliepen Oct 24 '20 at 21:06
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for answering. I've not noticed that std::variant<double> testNumber = 1; doesn't compile in GCC and I just tested that it can be compiled in MSVC v19.27. godbolt.org/z/9Trv9z \$\endgroup\$ – JimmyHu Oct 25 '20 at 0:27
  • 1
    \$\begingroup\$ @JimmyHu Protip: Clang on Godbolt uses libstdc++ by default (because Linux). If you want to test the LLVM project's libc++, you have to pass -stdlib=libc++, like so. (It also fails to compile with libc++.) The cpplang Slack tells me that this is known fallout from P0608, and so it's MSVC that is lagging the other vendors here. \$\endgroup\$ – Quuxplusone Oct 25 '20 at 0:40

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.