0

The keyGenerator should check which data type is given as an argument and convert it to an integer. It works for float and double, but when trying to convert a string to an integer, it does not work. Do you have any ideas why this might be happening?

#include <iostream>
#include <typeinfo>
#include <string>
#include <cmath>
using namespace std;

template<typename T>
class KeyValueKnot
{
public:
    T value;
    int key;
    KeyValueKnot *next;

    KeyValueKnot(T value)
    {
        this->value = value;
        this->key = keyGenerator(value);
        this->next = NULL;
    }
    int keyGenerator(T value)
    {
        if (typeid(value) == typeid(std::string))
        {
            return (int) generateNumberFromString(value);
        }
        else if (typeid(value) == typeid(double))
        {
            return static_cast<int>(value);
        }
        else if (typeid(value) == typeid(int))
        {
            return value;
        }
        else
        {
            std::cout << "Unknown type" << std::endl;
        }
        
    }
    
    int generateNumberFromString(string str)
    {
        int result = 0;
        for (char c : str)
        {
            result += static_cast<int>(c);
        }
        return result;
    }
    
};
user4581301
  • 33,082
  • 7
  • 33
  • 54
Anas
  • 11
  • 2
  • Because `float`s and `double`s implicitly convert to to `int`. `std::string` does not. Converting a string to an `int` has measurable cost and many failure cases, so the compiler forces you to acknowledge that you wish to pay this cost and have code in place to handle the failure cases. – user4581301 May 03 '23 at 18:45
  • 1
    The conditions have to be spelled as [`else`] `if constexpr (std::is_same_v)`, otherwise all brances will be validated even if they're never taken. The fallback branch should use [this](https://stackoverflow.com/a/56466352/2752075) instead of throwing an exception. – HolyBlackCat May 03 '23 at 18:45
  • `if (typeid(value) == typeid(std::string))` -- `else if (typeid(value) == typeid(double))` -- You cannot use runtime checks like this to check for the template type. Using these typese of checks does not do a compile-time branching of the code. – PaulMcKenzie May 03 '23 at 18:47
  • 1
    Disagree. You can do runtime checks like this, but it's generally better if the compiler does the job at compile time and can optimize out the now-dead code for the other cases. – user4581301 May 03 '23 at 18:49
  • Side note: when a function promises to return a value it must ALWAYS return a value. The `else` case does not return a value and invokes [undefined behaviour](https://en.cppreference.com/w/cpp/language/ub). The program may not function as intended. Worse, it may function as intended during your testing and then raise an army of vengeful machines against humanity shortly after deployment. – user4581301 May 03 '23 at 18:52
  • 1
    Your C++ textbook will explain how to specialize templates. This is the correct way to do something like what the shown code is trying to do, cleaner and without the ugly `if` mucking with typeids. – Sam Varshavchik May 03 '23 at 18:53
  • 1
    @user4581301 I would think as soon as you pass a `double` into the template, the compiler will trip up on the `generateNumberFromString`, because all branches will be evaluated. – PaulMcKenzie May 03 '23 at 18:54
  • @PaulMcKenzie Yes, you have me there in this case and probably most cases. The checks are fine, what comes after will fail. – user4581301 May 03 '23 at 18:58
  • [Yes, a compiler error when double is used](https://godbolt.org/z/8M93KhK8q). – PaulMcKenzie May 03 '23 at 19:01
  • @OP -- *It works for float and double*, No it doesn't work anymore. Try `double` again after adding the `generateNumberFromString` code. It looks like you didn't test `double` after making the coding change. Having said that, I wonder if there is a duplicate for this type of error, since you are not the first person to attempt to use `if/else` and `typeid`, hoping the compiler will choose the correct branch at compile-time. – PaulMcKenzie May 03 '23 at 19:02

1 Answers1

1

Your ifs are evaluated at runtime, so all branches need to be valid at compile-time.

However, when T is not std::string then the expression generateNumberFromString(value) is not valid for any type of T that is not implicitly convertible to std::string.

And, when T is std::string, the expressions static_cast<int>(value) and return value; are not valid because std::string can't be converted to int.

To fix this, you need to use if constexpr instead (or SFINAE or template specializations if you are using a pre-C++17 compiler), so the if branches are evaluated at compile-time instead, allowing unused branches to be removed by the compiler, eg:

#include <type_traits>

template<typename T>
inline constexpr bool always_false_v = false;

int keyGenerator(T value)
    {
        if constexpr (std::is_same_v<T, std::string>)
        {
            return (int) generateNumberFromString(value);
        }
        else if constexpr (std::is_same_v<T, double>)
        {
            return static_cast<int>(value);
        }
        //else if constexpr (std::is_same_v<T, int>)
        else if constexpr (std::is_convertible_v<T, int>) 
        {
            return value;
        }
        else
        {
            static_assert(always_false_v<T>, "Unknown type");
        }
    }

See How can I create a type-dependent expression that is always false? for an explanation of why always_false_v is needed in the static_assert (hint: without it, the code will always fail with an assert error!).

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770