OleksandrKvl / sbepp

C++ implementation of the FIX Simple Binary Encoding
https://oleksandrkvl.github.io/sbepp/
MIT License
42 stars 4 forks source link

Function to get the enum entry description #38

Closed ujos closed 5 months ago

ujos commented 6 months ago

Could you extend the sbeppc generator with the option to generate the function to return the description of the arbitrary enum value:

        <enum name="ErrorCode" encodingType="uint16">
            <validValue name="Other" description="Other">0</validValue>
            <validValue name="TooManyConnections" description="Amount of connected clients on the server reached the limit">1</validValue>
        </enum>
    constexpr char const* description(ErrorCode v) noexcept;
    {
        switch(v)
        {
            case ErrorCode::Other:
                return sbepp::enum_value_traits<ErrorCode::Other>::description();
          // ...
        }

        return "<undefined>";
    }
ujos commented 6 months ago

BTW, it seems it is not really good idea to define the types inside the detail namespace, as in order to make ADL work, I have to put the description() function into my_protocol::detail::types namespace.

namespace my_protocol::detail::types
{
    constexpr char const* description(my_protocol::types::ErrorCode v) noexcept
    {
        //...
    }
}

auto constexpr x = description(my_protocol::types::ErrorCode::Other);
ujos commented 6 months ago

And something is not right with the namespaces. In case if I want to extend the SBE parser with custom functionality (like my own implementation of the description method), then my code is getting bloated with namespaces (my_protocol::types, my_protocol::details::types, my_protocol::schema::types):

namespace my_protocol::detail::types
{
    constexpr char const* description(my_protocol::types::ErrorCode v) noexcept
    {
        switch(v)
        {
            case my_protocol::types::ErrorCode::Other:
                return sbepp::enum_value_traits<my_protocol::schema::types::ErrorCode::Other>::description();
            case my_protocol::types::ErrorCode::TooManyConnections:
                return sbepp::enum_value_traits<my_protocol::schema::types::ErrorCode::TooManyConnections>::description();
        }

        return "<undefined>";
    }

}
OleksandrKvl commented 6 months ago

Could you extend the sbeppc generator with the option to generate the function to return the description of the arbitrary enum value

Definitely not in the way you showed above. But I'll think how to make it more generic to provide similar functionality.

BTW, it seems it is not really good idea to define the types inside the detail namespace, as in order to make ADL work, I have to put the description() function into my_protocol::detail::types namespace.

First of all, it's not a good idea to add anything into a third-party namespaces, especially into detail. Second, relying on ADL using unqualified function calls is a very strong code-smell. You can have that function anywhere in your own namespace and it will work perfectly fine.

And something is not right with the namespaces.

They are perfectly fine, do you really think that I deliberately added random namespaces into the overall structure to make it more complex?

In case if I want to extend the SBE parser with custom functionality

Then do it in your own namespace, don't hack into external ones.

ujos commented 6 months ago

I have a question, why do you define the class for the every enum entry. Enum itself can be a template parameter:

enum SomeEnum { A, B};

template<SomeEnum E>
struct traits;

template<>
struct traits<SomeEnum::A>
{
   static inline constexpr char const* name = "A";
};

template<>
struct traits<SomeEnum::B>
{
   static inline constexpr char const* name = "B";
};

Following are my comments to your message.

BTW, it seems it is not really good idea to define the types inside the detail namespace

Another drawback is error message which is hard to understand. Consider the following artificial code:

void foo(ErrorCode, OptionType) {}

void bar()
{
    foo(OptionType::Call, ErrorCode::TooManyConnections);
}

If you try to compile it, you get:

error: cannot convert ‘my_protocol::detail::types::enum_3’ to ‘my_protocol::types::ErrorCode 
  {aka my_protocol::detail::types::enum_1}’ for argument ‘1’ 
  to ‘void foo(my_protocol::types::ErrorCode, my_protocol::types::OptionType)’

Second, relying on ADL using unqualified function calls is a very strong code-smell.

I'm not sure. Do you have an explanation why ADL is bad? The idea behind the ADL is to be able to call the functions using unqualified function calls

You can have that function anywhere in your own namespace and it will work perfectly fine.

What if I have multiple namespaces? Then in order to call the function using unqualified function calls I have to copy that function to every namespace. Or define it in the global namespace, which is not good.

First of all, it's not a good idea to add anything into a third-party namespaces, especially into detail.

Well, I'm not defining the code in the third party namespace. The my_protocol is my namespace, isn't it? I just want to extend my parser with extra functionality.

do you really think that I deliberately added random namespaces into the overall structure to make it more complex?

Of course not. What I say is the way how namespaces are organized causes conflicts, so developer is forced to use fully qualified namespaces which makes code hard to read and understand.

OleksandrKvl commented 6 months ago

I have a question, why do you define the class for the every enum entry. Enum itself can be a template parameter

That would be inconsistent design because everything else in XML schema is reachable through schema_name::schema. But I'm thinking about such mapping (#41).

Do you have an explanation why ADL is bad? The idea behind the ADL is to be able to call the functions using unqualified function calls

You can just google "C++ ADL problem" and find tons of questions, problems, articles, explanations how it works and even attempts to fix it. It is not a mechanism one should use on a daily basis just to make the code shorter.

What if I have multiple namespaces? Then in order to call the function using unqualified function calls I have to copy that function to every namespace. Or define it in the global namespace, which is not good.

Define a function ::sbe_utils::describe(types::MyEnum) and then use its qualified name everywhere. What's the problem?

The my_protocol is my namespace, isn't it?

No, SBE XML is yours, the generated code is not. It's an instantiation of very large C++ template to which XML content is passed as input, you're allowed to use it only through public documented interface, otherwise all bets are off. std::vector<T> is not yours just because you own T and when you need a function that works on std::vector, you don't write it inside std namespace.

What I say is the way how namespaces are organized causes conflicts, so developer is forced to use fully qualified namespaces which makes code hard to read and understand.

Using qualified names is the way to write C++ code. I use unqualified names only when they refer to the enclosing namespace, all other C++ projects I know do the same. Again, what conflicts? What you showed above is not a conflict, you're just trying to hack into the generated code and surprised that it's not convenient. The solution here is very simple, write your function in your own namespace and call it from anywhere you want.

OleksandrKvl commented 5 months ago

@ujos with #48 merged, you can now implement requested functionality like:

struct enum_visitor{
    template<typename Enum, typename Tag>
    void on_enum_value(Enum /*e*/, Tag){
        description = sbepp::enum_value_traits<Tag>::description();
    }

    template<typename Enum>
    void on_enum_value(Enum /*e*/, sbepp::unknown_enum_value_tag){
        description = nullptr;
    }

    const char* description;
};

// returns `nullptr` if `e` is unknown
template<typename Enum>
const char* description(Enum e){
    return sbepp::visit<enum_visitor>(e).description;
}

Closing as completed, let me know if you need more support with this.