2
\$\begingroup\$

In a project, we have a file that contains something like a script which we need to parse. At the moment the validation is tough since we have an array of functions returing variable and taking variableList (basically std::variant<>and vector of variants, see definition below)
I am trying to have function metadata available (number of input arguments and its type, possibly return value type for validating the input "script". I would like to replace std::function with something like this:

using variable = std::variant<bool, int, double, std::string>;
using variableList = std::vector<variable>;

enum {
    BOOL_TYPE = 0,
    INT_TYPE = 1,
    DOUBLE_TYPE = 2,
    STRING_TYPE = 3,

    VARIABLE_BOOL_TYPE = 0 | 0x80,
    VARIABLE_INT_TYPE = 1 | 0x80,
    VARIABLE_DOUBLE_TYPE = 2 | 0x80,
    VARIABLE_STRING_TYPE = 3 | 0x80
};

struct Function
{
    std::string                                 fname;
    std::vector<int>                            indexes;
    std::function<variable(variableList &&)>    fn;

    variable operator()(variableList && list)
    {
        if (indexes.size() != list.size() &&
            (indexes.size() == 0 || (indexes[0] & 0x80) != 0x80))
        {
            std::cout << "Invalid number of arguments" << std::endl;
            return variable(0);
        }


        int idx = 0;
        for (auto i : list)
        {
            if ((indexes[0] & 0x80) ? i.index() != (indexes[0] & 0x03), idx++ : i.index() != indexes[idx++])
            {
                std::cout << "Invalid argument type" << std::endl;
                return variable(0);
            }
        }

        return fn(std::forward<decltype(list)>(list));
    }
};

int main()
{
    Function f = { "fName", {BOOL_TYPE, INT_TYPE, BOOL_TYPE}, [](variableList && list) -> variable {
        for (auto i : list)
        {
            std::visit([](auto && val) -> void {
                std::cout << val << std::endl;
            }, i);
        }
        return variable(0);
    }};

    auto arguments = {variable(true),variable(true),variable(true)};
    auto returnValue = f(arguments);
}

The project supports c++17. Any modifications, suggestions are welcome.

EDIT: Initially I wanted to use templates instead of indices and enum, but I wasn't quite successful with creating array of Function class with different template arguments.

\$\endgroup\$
2
\$\begingroup\$

Prefer enum class with a name, and probably rename the enums, e.g. BOOL_TYPE becomes TYPE::BOOL or however you'd like to do the naming. Why do you want them to be an unnamed enum type?


You're printing an error to cout when there's an error. At the very least, print to cerr, so that someone running this might be able to filter output if they'd like to. I would personally either return a variant<variable, error_message_type> or throw an exception if there's an error like that though. Otherwise errors could easily slip through unnoticed.


This validation loop is inefficient and confusing.

Prefer extracting the conditional if possible. It appears to be ensuring that all the types are the same if it is a "VARIABLE_" type. Perhaps "variadic" may be more appropriate?

    int idx = 0;
    for (auto i : list)
    {
        if ((indexes[0] & 0x80) ? i.index() != (indexes[0] & 0x03), idx++ : i.index() != indexes[idx++])
        {
            std::cout << "Invalid argument type" << std::endl;
            return variable(0);
        }
    }

indexes could take a type enum instead of int if it had a name. Why is it called indexes? You may be able to leverage more of std::variant and possibly not need the enum at all.


for (auto i : list) You probably want auto & i, since this will result in string copies in the case of string variants.


return fn(std::forward<decltype(list)>(list)); This is unnecessary. std::forward is useful for templated types, but you know the type of list, it's a variableList &&.

You could move the list, but I'm not sure that's what you really want.

\$\endgroup\$
2
  • \$\begingroup\$ I totally agree with all the above, but I didn't want this question to be about such things but rather how to store that information in a more viable manner. - Something like std::tuple with checks whether an array of variants have the same type as std::tuple variadic parameters in that order \$\endgroup\$ – Quest Jan 16 '20 at 21:55
  • \$\begingroup\$ A tuple of types as the signature would require the Function class to be templated (and heterogenous). It could work but it would need to inherit from some base class if you wanted to store them in a container, and it would generate a new set of validation code per function signature. Otherwise a list of enums is fine, you could loop over them and switch on the enum value, testing the argument type using std::holds_alternative \$\endgroup\$ – butt Jan 16 '20 at 23:00
1
\$\begingroup\$

What do you think of this?

Add script functions to a function map like this:

function_map["function name"] =
      MakeScriptFunction<ReturnType, ArgumentTypes...>(fn);

call them like this:

//                  |arg 0       |arg 1|arg 2| arg 3|
function_map["foo"]({"foo string", 69.0,  666, false});

// or like this
function_map["foo"](variable_list);

Demo that shows off construction, calling, and what happens if wrong argument numbers or types are provided. Note that you can do a non-exception implementation using sizeof...(ArgumentTypes) and std::holds_alternative. https://godbolt.org/z/sdgMra

#include <functional>
#include <iostream>
#include <unordered_map>
#include <variant>

using Variable = std::variant<bool, int, double, std::string>;
using VariableList = std::vector<Variable>;
using ScriptFunction = std::function<Variable(VariableList const&)>;

template <typename ReturnType, typename... ArgumentTypes,
          typename FunctionType = std::function<ReturnType(ArgumentTypes&&...)>>
ScriptFunction MakeScriptFunction(FunctionType&& function) {
  return [function = std::move(function)](VariableList const& argument_list) {
    auto argument_iter = argument_list.rbegin();
    return function(std::get<ArgumentTypes>(*argument_iter++)...);
  };
}

int main() {
  auto function_map = std::unordered_map<std::string, ScriptFunction>{};

  function_map["foo"] = MakeScriptFunction<int, std::string, double, int, bool>(
      [](auto& s, auto d, auto i, auto b) {
        std::cout << "inside foo, s=" << s << " d=" << d << " i=" << i
                  << " b=" << b << std::endl;
        return 420;
      });
  function_map["bar"] =
      MakeScriptFunction<std::string>([]() { return "barbarbar"; });

  // bad definition (mismatch of function and declared types) causes compile
  // error error: no match for call to '(main()::<lambda()>) (const int&)' 16 |
  // return function(std::get<ArgumentTypes>(*argument_iter++)...);
  //   function_map["bad definition"] =
  //       MakeScriptFunction<std::string, int>([]() { return "barbarbar"; });

  // inline call
  function_map["foo"]({"foo string", 69.0, 666, false});

  // l value call
  auto foo_args = VariableList{"foo string", 69.0, 666, false};
  function_map["foo"](foo_args);

  // print out return value of a function
  std::visit([](auto&& v) { std::cout << v << std::endl; },
             (function_map["bar"])({}));

  // wrong order
  try {
    function_map["foo"]({"foo string", 69.0, false, 666});
  } catch (std::exception& e) {
    std::cerr << e.what() << std::endl;
  }

  // wrong number of args
  try {
    function_map["foo"]({"foo string", 69.0, 666});
  } catch (std::exception& e) {
    std::cerr << e.what() << std::endl;
  }
  return 0;
}

\$\endgroup\$
1
  • \$\begingroup\$ this has a bug if too many parameters are supplied but can be addressed with size... \$\endgroup\$ – butt Jan 17 '20 at 2:43
0
\$\begingroup\$

I ended up with something like this:

template<std::size_t N, typename T>
inline std::enable_if_t<N == 0, bool> typeCheck(variableList& list) noexcept
{
    if (false == std::holds_alternative<std::tuple_element_t<N, T>>(list[N]))
    {
        std::cout << "Function type check failed for " << N << ". element." << std::endl;
        std::cout << "Expected: " << typeid(std::tuple_element_t<N, T>).name() << "; Got: " <<
            std::visit([](auto&& t) -> std::string { return typeid(decltype(t)).name(); }, list[N]) << std::endl;
        return false;
    }
    return true;
}

template<std::size_t N, typename T>
inline std::enable_if_t<0 < N, bool> typeCheck(variableList& list) noexcept
{
    if (false == std::holds_alternative<std::tuple_element_t<N, T>>(list[N]))
    {
        std::cout << "Function type check failed for " << N << ". element." << std::endl;
        std::cout << "Expected: " << typeid(std::tuple_element_t<N, T>).name() << "; Got: " <<
            std::visit([](auto&& t) -> std::string { return typeid(decltype(t)).name(); }, list[N]) << std::endl;
        return false;
    }
    return typeCheck<N-1, T>(list);
}

struct Function
{
    virtual std::size_t numArguments() const noexcept = 0;
    virtual bool checkVariables(variableList& list) const noexcept = 0;
    virtual variable operator()(variableList& list) = 0;
};

template<typename... Ts>
struct FunctionImpl : Function
{
    using pack = std::tuple<Ts...>;

    FunctionImpl(std::function<variable(Ts && ...args)>&& fn) : Function{}, fn(std::forward<decltype(fn)>(fn)) {}

    variable operator()(variableList& list)
    {
        return std::apply(fn, vectorToTuple<std::tuple_size_v<pack>>(list));
    }

    std::size_t numArguments() const noexcept
    {
        return std::tuple_size_v<pack>;
    }
    bool checkVariables(variableList& list) const noexcept
    {
        if (list.size() < std::tuple_size_v<pack>)
        {
            std::cout << "Bad number of arguments" << std::endl;
            return false;
        }

        return typeCheck<std::tuple_size_v<pack> -1, pack>(list);
    }

private:
    std::function<variable(Ts && ...args)> fn;

    template <typename T, typename std::size_t... Indices>
    auto vectorToTupleHelper(const std::vector<T>& v, std::index_sequence<Indices...>) {
        return std::make_tuple(std::get<std::tuple_element_t<Indices, pack>>(v[Indices])...);
    }

    template <std::size_t N, typename T>
    auto vectorToTuple(const std::vector<T>& v) {
        return vectorToTupleHelper(v, std::make_index_sequence<N>());
    }
};

#include <memory>
std::array<std::unique_ptr<Function>, 2> fnArray = {
    std::make_unique<FunctionImpl<bool,int>> ([](bool,int) -> variable {
        return variable(0);
    }),
    std::make_unique<FunctionImpl<int, double, bool>> ([](int a, short b, bool c) -> variable {
        std::cout << "    a: " << a << std::endl
            << "    b: " << b << std::endl
            << "    c: " << c << std::endl;
        return variable(12);
    })
};

int main()
{
    variableList list = { variable(2), variable(2.2), variable(false) };
    std::cout << std::boolalpha << fnArray[0]->checkVariables(list) << std::endl;
    std::cout << std::boolalpha << fnArray[1]->checkVariables(list) << std::endl;

    std::cout << "Calling 2:" << std::endl;
    auto result = fnArray[1]->operator()(list);
    std::cout << "    result: " << std::get<int>(result) << std::endl;

    return 0;
}
\$\endgroup\$
3
  • \$\begingroup\$ list.size() < std::tuple_size_v<pack> Why not do an equality comparison? It's possible that this will accidentally work, i.e. an unintended extra argument wouldn't cause an error where you would want it to. \$\endgroup\$ – butt Jan 17 '20 at 22:31
  • \$\begingroup\$ Oh, yea sure, ty nice catch \$\endgroup\$ – Quest Jan 17 '20 at 22:38
  • \$\begingroup\$ I don't think the T parameter is needed in vectorToTuple. Also you could use a templated lambda so you don't need a helper function. Templated lambdas are only in gcc right now though I think. \$\endgroup\$ – butt Jan 17 '20 at 22:48
0
\$\begingroup\$

Fix the warnings

235745.cpp: In member function ‘variable Function::operator()(variableList&&)’:
235745.cpp:43:49: warning: value computed is not used [-Wunused-value]
   43 |             if ((indexes[0] & 0x80) ? i.index() != (indexes[0] & 0x03), idx++ : i.index() != indexes[idx++])
      |                                       ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
235745.cpp:43:91: warning: comparison of integer expressions of different signedness: ‘std::size_t’ {aka ‘long unsigned int’} and ‘__gnu_cxx::__alloc_traits<std::allocator<int>, int>::value_type’ {aka ‘int’} [-Wsign-compare]
   43 |             if ((indexes[0] & 0x80) ? i.index() != (indexes[0] & 0x03), idx++ : i.index() != indexes[idx++])

It looks like the first of those is reporting a serious error; perhaps that line was supposed to be:

        if ((indexes[0] & 0x80) ? ++idx, i.index() != (indexes[0] & 0x03) : i.index() != indexes[idx++])

Or just start idx at -1, and increment it immediately before the if instead.

Magic numbers

There's a sprinkling of 0x80 and 0x3 around the code with no explanation. I'd expect those to be named constants.

Default promotions and default arguments.

This seems much more restrictive than standard function calls, where (for example) int arguments can be promoted for functions expecting double. Is that intentional? It's certainly surprising.

Similarly, it's disappointing that we can't use default arguments as we can in ordinary C++ functions.

Formatting

Please don't do this - it's really hard to read:

std::string                                 fname;
std::vector<int>                            indexes;
std::function<variable(variableList &&)>    fn;
\$\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.