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;
};
5 Answers 5
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)
-
\$\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– alexander rose2020年03月14日 03:57:26 +00:00Commented Mar 14, 2020 at 3:57
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.
-
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– Deduplicator2020年03月14日 14:26:32 +00:00Commented Mar 14, 2020 at 14:26
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";
}
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.
-
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– Roman Odaisky2020年03月15日 20:30:42 +00:00Commented Mar 15, 2020 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.– Matthieu M.2020年03月16日 09:59:59 +00:00Commented Mar 16, 2020 at 9:59
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.
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.
type()
returns atype_info const &
, you can compare that totypeid(std::string)
to find out if it's a string. \$\endgroup\$