7
\$\begingroup\$

I have created a Node class that I am using for various projects in my data structures class(at school) and I am overloading the insertion operator <<. I am using std::any for my container datatype inside Node class and am using std::any_cast to insert this data into a stream whenever possible. Std::any_cast requires that I know the datatype beforehand, and so I've come up with a hacky way to get this information by detangling the value typename into a string and then using a ton of else if statements to compare this string, as you can see in my code below... I'm looking for a different solution or even some advice about why this approach is fundamentally wrong and how to go about making better programming decisions. Lay it on me, I need help and can take it!

friend std::ostream& operator << (std::ostream& out, Node& n){

    std::string t;
    t = abi::__cxa_demangle(n.value.type().name(), 0, 0, 0);;

    if (t.find("string") != std::string::npos) { out << std::any_cast<std::string>(n.value); }
    else if (t == "unsigned long long int") { out << std::any_cast<unsigned long long int>(n.value); }
    else if (t == "unsigned long int") { out << std::any_cast<unsigned long int>(n.value); }
    else if (t == "long long int") { out << std::any_cast<long long int>(n.value); }
    else if (t == "long int") { out << std::any_cast<long int>(n.value); }
    else if (t == "unsigned short int") { out << std::any_cast<unsigned short int>(n.value); }
    else if (t == "short int"){ out << std::any_cast<short int>(n.value); }
    else if (t == "int") { out << std::any_cast<int>(n.value); }
    else if (t == "long double") { out << std::any_cast<long double>(n.value); }
    else if (t == "double") { out << std::any_cast<double>(n.value); }
    else if (t == "float") { out << std::any_cast<float>(n.value); }
    else if (t == "bool") { out << std::any_cast<bool>(n.value); }
    else if (t == "unsigned char") { out << std::any_cast<unsigned char>(n.value); }
    else if (t == "signed char") { out << std::any_cast<signed char>(n.value); }
    else if (t == "char") { out << std::any_cast<char>(n.value); }
    else { out << t; };

    return out;
};

\$\endgroup\$
  • 2
    \$\begingroup\$ Seems like you missed the class on inheritance. \$\endgroup\$ – Martin York Mar 13 at 22:57
  • 3
    \$\begingroup\$ Welcome. This here is CodeReview@SE, set to help improve working code from a project as well as coding habits and skills. I do not see the code presented to be part of any judicious attempt of tackling a problem. Choosing among the mechanisms of a programming language of choice seems a better fit over at StackOverflow. \$\endgroup\$ – greybeard Mar 14 at 7:13
  • \$\begingroup\$ @greybeard my mistake, apologies for misunderstanding. I can't seem to remove this post \$\endgroup\$ – alexander rose Mar 14 at 9:26
  • \$\begingroup\$ (No longer thinking a post was the best course of action is not a sufficient cause to remove it: thinking it not helpful is. With questions, this includes all the answers…) \$\endgroup\$ – greybeard Mar 14 at 9:49
  • 1
    \$\begingroup\$ If type() returns a type_info const &, you can compare that to typeid(std::string) to find out if it's a string. \$\endgroup\$ – Simon Richter Mar 14 at 11:14
10
\$\begingroup\$

First, this call to __cxa_demangle returns a buffer allocated with malloc and which must be freed. This isn't freed, so this code has a memory leak.

Second, as I understand it, __cxa_demangle does not exist in MSVC, so this is not portable.

Third, the .find("string") catches more than std::string. It would match std::wstring, std::stringstream, void (*)(std::string).

Fourth, this code relies essentially on knowing the potential types that are in that std::any. That is knowledge at a distance, and if you allow more types at the place of construction, you need more types here.

Fifth, if the type isn't one of those types, the error is silent and the output is the type contained, not the value. That could be harmless, but it could be a real problem. If you allowed type Foo in the any, and neglected to update this location, you wouldn't be able to tell if the output Foo was that type name or the value of a string stored in the any.

The suggestion of the other answer is to constrain the types stored using the type storing them. Instead of any use std::variant<std::string, long long, unsigned long long, double>. I added the string and unsigned options relative to the other answer. Then just visit:

std::visit([&](const auto& value) { out << value; }, n.value);

This is a very strong option.

A second option would be to erase not almost every aspect of the type (as with std::any) but instead leave the interface you need, e.g. erase everything but the stream operator if that's all you need:

    printable p = 2.3;
    std::cout << p << "\n";
    p = "foo";
    std::cout << p << "\n";

Example: https://godbolt.org/z/NtJqWk

(edit: added a missing virtual destructor to impl_base)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your feedback, Im getting way too ahead of myself and ended up stripping most everything away. I have been going though each one of your points and taking note. I appreciate your time and thanks for the great response! \$\endgroup\$ – alexander rose Mar 14 at 3:57
7
\$\begingroup\$

There are way too many ways to improve the situation. Some require big changes some require small changes.

Why not simply store the data inside double instead of any? double only fails to 100% accurately contain long long types - but even that happens for very large numbers. At most have long long, double and a boolean that indicates which variable it is. Or say use std::variant<double, long long>.

Using std::any for basic types is a total overkill and will surely result in lots of overhead.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ You overlooked the first option: std::string. Aside from that, yes, restricting the options, and potentially merging some of them, is a good idea. \$\endgroup\$ – Deduplicator Mar 14 at 14:26
4
\$\begingroup\$

Other answers have already provided alternative solutions to std::any, such as std::variant, or inheritance; this answer will focus on correct usage of std::any itself.

and so I've come up with a hacky way to get this information by detangling the value typename into a string and then using a ton of else if statements to compare this string

std::any does actually provide several ways to query the underlying type without relying on string parsing, let's review them below:

void ingest_any(std::any any)
{
    try
    {
        std::cout << std::any_cast<std::string>(any) << "\n";
    }
    catch (std::bad_any_cast const&) {}

    if (std::string* str = std::any_cast<std::string>(&any)) {
       std::cout << *str << "\n";
    }

    if (std::type_index{ typeid(std::string) } == any.type()) {
       //  Known not to throw, as previously checked.
       std::cout << std::any_cast<std::string>(any) << "\n";
    }
}

In general, when the actual type is not known, I would suggest either relying on the pointer form of any_cast for ad-hoc querying, or on type_index for look-ups.

When you are about to create an if-ladder, you are in the look-up case:

using AnyAction = std::function<void(std::any const&)>;

template <typename T>
void print(std::any const& a) { std::cout << std::any_cast<T>(a) << "\n"; }

std::map<std::type_index, AnyAction> const Actions = {
    { typeid(std::string), print<std::string> },
    { typeid(unsigned long), print<unsigned long> },
    { typeid(long), print<long> },
    { typeid(double), print<double> },
    { typeid(float), print<float> },
    { typeid(char), print<char> },
};

void doit(std::any const& a)
{
    if (auto it = Actions.find(a.type()); it != Actions.end())
    {
        it->second(a);
        return;
    }

    std::cout << "Unknown type: " << a.type().name() << "\n";
}

See it in action on godbolt.

The main advantage of such maps is that they need not be const; you can instead use a run-time registry in which the user registers a type, as well as the appropriate (various) actions that can be undertaken on that type.

This allows a core code which knows nothing of the concrete types, and therefore allows a library never to expose its internal types to the exterior world.

Of course, in exchange for the flexibility, you pay a run-time penalty.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ using AnyAction = void (*)(std::any const &); would work just as well but with less indirection so with slightly better performance. \$\endgroup\$ – Roman Odaisky Mar 15 at 20:30
  • \$\begingroup\$ @RomanOdaisky: In this very specific case, yes. As soon as you'd like to capture some state, it would stop working though. \$\endgroup\$ – Matthieu M. Mar 16 at 9:59
2
\$\begingroup\$

You can avoid all the copypasta by writing a template function that tries a number of types in a row. The idea is that e. g. visit_any_as<int, double, char>(any, visitor) will try calling visitor(std::any_cast<int>(any)) if possible, otherwise it will call visit_any_as<double, char>(any, visitor) to try the next type, and so on until the types are exhausted:

#include <any>
#include <iostream>
#include <stdexcept>
#include <string>
#include <utility>

template <class Visitor>
void visit_any_as(std::any const &, Visitor &&)
{
    throw std::logic_error("std::any contained no suitable type, unable to visit");
}

template <class First, class... Rest, class Visitor>
void visit_any_as(std::any const& any, Visitor&& visitor)
{
    First const* value = std::any_cast<First>(&any);
    if(value)
    {
        visitor(*value);
    }
    else
    {
        visit_any_as<Rest...>(any, std::forward<Visitor>(visitor));
    }
}

int main()
{
    std::any any{-1LL};

    try
    {
        visit_any_as<std::string, int, double, char>(any, [](auto const& x) {
            std::cout << x << std::endl;
        });
    }
    catch(std::exception const& e)
    {
        std::cout << e.what() << std::endl;
    }
}

However, do consider std::variant if you know in advance the list of possible types, as then std::visit will solve the problem much more neatly.

| improve this answer | |
\$\endgroup\$
-2
\$\begingroup\$

You kept it concise. Anyone concerned with your code can easily comment it out and rewrite it. Add a test case and everyone will help you fix stuff if it needs it.

| improve this answer | |
\$\endgroup\$

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.