2
\$\begingroup\$

I created a simple enum macro that creates a to_string method for the enum. This has been done before, but my version is designed to compile quickly. From what I can tell, existing libraries use deep preprocessor macros and lots of template instantiations which is taxing for large enums.

Here is the code:

#include <array>
#include <string_view>

#define EnumWithUnderlying(Name, NTy, ... )\
enum class Name : NTy {__VA_ARGS__, End };\
namespace std {\
    std::string_view to_string(enum Name enumIn) noexcept {\
        constexpr static string_view fullstr = #__VA_ARGS__ ", End ";\
        struct BeginLen {\
            std::size_t begin;\
            std::size_t len;\
        };\
        using ArrIdxTy = std::array<BeginLen, NTy(Name::End)+1>;\
        constexpr static ArrIdxTy begins = [&]() {\
            ArrIdxTy ret = {};\
            std::size_t jbegin = 0;\
            std::size_t jend = 0;\
            enum ParserState {Before,In,After};\
            ParserState state = Before;\
            for (std::size_t i = 0; i < fullstr.size(); ++i) {\
                auto isSpace = [](char ch) -> bool {\
                    return ch == ' ' || ch == '\t' || ch == '\n';\
                };\
                switch (state) {\
                    case Before:\
                        if (!isSpace(fullstr[i])) {\
                            ret[jbegin].begin = i;\
                            jbegin++;\
                            state = In;\
                        } else {\
                            break;\
                        }\
                    case In:\
                        if (isSpace(fullstr[i]) || fullstr[i] == ',') {\
                            ret[jend].len = i - ret[jend].begin;\
                            jend++;\
                            state = fullstr[i] == ',' ? Before : After;\
                        } else {\
                            break;\
                        }\
                        break;\
                    case After:\
                        if (fullstr[i] == ',') {\
                            state = Before;\
                        }\
                        break;\
                }\
            }\
            return ret;\
        }();\
        using ArrStrTy = std::array<std::string_view, NTy(Name::End)+1>;\
        constexpr static ArrStrTy strs = [&]() {\
            ArrStrTy ret = {};\
            for (std::size_t i = 0; i < begins.size(); ++i) {\
                ret[i] = std::string_view(fullstr.begin() + begins[i].begin, begins[i].len);\
            }\
            return ret;\
        }();\
        return strs[NTy(enumIn)];\
    };\
}
#define Enum(Name,...) EnumWithUnderlying(Name, int, __VA_ARGS__)

Example usage:

#include <iostream>
#include "enum.h"

EnumWithUnderlying(Move, char,
  UP,
  DOWN,
  LEFT,
  RIGHT
);

Enum(Letter, A, B);

int main() {
std::cout << 
    std::to_string(Move::UP) << " " <<
    std::to_string(Move::DOWN) << " " <<
    std::to_string(Move::LEFT) << " " <<
    std::to_string(Move::RIGHT) << " " <<
    int(Move::End) << " " <<
    std::to_string(Letter::A) << " " <<
    std::to_string(Letter::B) << " " <<
    std::endl;
}

I'm open to any comments, but I don't want to do anything that makes it take longer to compile.

Also, I noticed that making to_string constexpr/getting rid of the static members seems to generate worse code. I don't fully understand why that is since I'd expect a constexpr version to be strictly faster.

For the record this is also on github

\$\endgroup\$
3
  • \$\begingroup\$ Just realized codereview.stackexchange.com/questions/38703/… is very similar. Feel free to answer but I'lm also going to read thru that \$\endgroup\$ Jun 12, 2020 at 2:54
  • 1
    \$\begingroup\$ Why did you include all those extraneous backslashes? Frankly, imho, it make your code look like a jumbled mess. \$\endgroup\$
    – user33306
    Jun 12, 2020 at 3:56
  • \$\begingroup\$ The backslashes allow the macro to be written on mutliple lines. I suppose I could factor out most of it into a function outside of the macro \$\endgroup\$ Jun 12, 2020 at 4:18

1 Answer 1

3
\$\begingroup\$

I’m really not a fan of this idea, on a lot of levels. First of all, it’s extremely limited; it only works for the most basic enum definitions. As soon as you add initializers or attributes, chaos ensues. Second, it’s just absurdly overcomplicated for what should be trivial code. The fact that you’re having problems with code gen should be a warning sign. There is no way I could justify the debugging effort if I tried to use this and something went sideways.

In my opinion, manually rolling out a to_string() function for an enum isn’t that much extra work. Certainly not enough to justify the level of complexity of this macro. And if your primary concern is compile time (really? that’s the most important thing? how many enums do you have, and how big are they that this is even measurable, let alone the most crucial issue?), then a manually rolled function will compile MUCH faster than the code in that macro. Yeah, sure, you have to repeat yourself a little bit… but not that much. Certainly not enough to justify a state machine parser for a string (that also only works in the simplest cases).

But okay, let me set aside my qualms about the overall idea, and focus on the code itself.

#define EnumWithUnderlying(Name, NTy, ... )\
enum class Name : NTy {__VA_ARGS__, End };\

You don’t include any comments or documentation to explain the logic of the macro, so I don’t know if this extra End enumerator is a feature or a bug. In other words, I don’t know if there is a reason someone might want End added to their enumerators, or if you just added it to make your code work.

Assuming it’s just there to make things work… the only purpose I can make out for it is that you use it to figure out the size of your enumerator arrays, such as in:

using ArrIdxTy = std::array<BeginLen, NTy(Name::End)+1>;

This “works” only so long as none of the enumerators have any initializers (that don’t happen to equal what their default values would be), but aside from being brittle, it’s also unnecessary. If all you need is the count of initializers, then all you need is something as simple as:

template <typename... Args>
constexpr auto count_args(Args&&...) noexcept
{
    return sizeof...(Args);
}

using ArrIdxTy = std::array<BeginLen, count_args(__VA_ARGS__) + 1>;

If End serves some other purpose, then fine, there’s no need to remove it… but in that case you still might want to get the number of enumerators using a count function like the one above. That way your macro won’t break if any enumerators have initialzers. (Well, it won’t break in that particular way, anyway.)

namespace std {\
    std::string_view to_string(enum Name enumIn) noexcept {\

There are a very limited number of cases where you are allowed to add stuff to std—really just a few templates that you are allowed to specialize in fairly restricted ways. Adding a completely new function? Definitely not allowed. No, not even if it’s an overload of an existing function.

To make matters worse, the function lies. When I call to_string()… I expect to get a string. Not a view to a string.

I figure that your goal is to be able to write std::to_string(enum_value)… but that goal is misguided. When I see std::some_func() that I’ve never seen before, the very first thing I do is go straight to my favourite standard library reference (cppreference) and read the docs. But I won’t find this function there, now will I?

You might respond: “But it does the same thing std::to_string() does for all the types it is defined for!” Does it, though? If I do auto x = Move::UP; std::cout << std::to_string(x);, it will print “UP”. But if I do auto x = SOME_INT_CONSTANT; std::to_string(x); it will not print “SOME_INT_CONSTANT”. std::to_string(int) prints the (locale-flavoured) value of an int… not its name… yet your enumerator-specific std::to_string() functions print the name. Not the same thing, now is it?

You might respond: “But what it does is so obvious!” Is it, though? I mean, ignoring that it actually doesn’t do what it claims to (it returns a std::string_view, not a std::string), converting an enumerator value to a string doesn’t necessarily mean returning the enumerator’s name. It might actually make more sense to return the enumerator’s numeric value as a string in some cases.

So in summary:

  • std::to_string() is the wrong name for this function; and
  • even if it weren’t, you can’t use it.

So what’s the right name? Well, why not just to_string() in the same namespace as the enum? (Well, better would be to_string_view(), but one thing at a time.) ADL would find the function when you need it, so you’d only have to call to_string(enum_value)… which is actually shorter than std::to_string(enum_value).

(It might also make sense to define a to_string() function in terms of a to_string_view() function. That’s actually how I usually do it.)

Now to get into the meat of the code… what this does (as I understand it) is take the variadic macro arguments, translate the bunch of them into a single string, then use a state machine to parse out the enumerator names into an array. As I mentioned above, your parser is too simplistic—it can’t handle initializers or attributes—but let’s ignore that issue for now.

The biggest problem with your strategy is the assumption that all the enumerators in an enumeration have their default “natural” values. If they don’t, you probably have UB, because your enumerator values won’t be valid indexes into the array.

A less critical issue is that if you really only want to support trivial enumerations (that is, no initializers), that’s fine… but in that case, this seems wildly over-complicated. All you need to do to find the Nth enumerator is find the (N−1)th comma and the Nth comma, take everything between them, then trim the whitespace. Special case the first and last enumerators (or don’t! add a leading and trailing comma to the string to simplify the algorithm!), and you’re done.

If you intend to support general enumerations, then this design just won’t work: you can’t simply cast the enumerator to a number and use that as an index. You need a map. No, not a std::map; that would be overkill. std::array<std::tuple<Name, std::string_view>> would suffice. To actually implement this, all you’d need is some kind of preprocessor for-each—either roll your own or use one from a library—then you could just do something like this:

// Takes an arbitrary enumerator string with arbitrary whitespace:
//     *   "foo"
//     *   "foo = N"
//     *   "foo [[attribute]]"
//     *   "foo [[attribute]] = N"
// and extracts just the enumerator ("foo").
constexpr auto extract_enumerator(std::string_view) -> std::string_view;

#define ENUM_STR_MAP_ITEM(enumerator) std::tuple{enumerator, extract_enumerator(#enumerator)},

constexpr auto enumerator_string_map = std::array{
    FOREACH(ENUM_STR_MAP_ITEM, __VA_ARGS__)
};

And to_string_view() could use that map.

struct BeginLen {\
    std::size_t begin;\
    std::size_t len;\
};\

So, your strategy is to parse the string into an array of BeginLen objects, then transform that into an array of std::string_view. But… why? A std::string_view is literally just a “begin” and a “len” already. The entire last quarter of your function is literally just a transform that does std::string_view{begin, len} for each BeginLen. Why not parse the enumerator strings directly into std::string_views and skip that last bit?

std::size_t jbegin = 0;\
std::size_t jend = 0;\

Why do you need two indexes to keep track of which enumerator you’re currently parsing? (I mean, I assume that’s what those variables are for, in lieu of any comments or even clear names.) Couldn’t you drop jend, remove the jbegin++;, and replace jend with jbegin in the In case?

Summary

You have a number of critical bugs and conformance issues:

  • You are not allowed to add new functions to namespace std. (An overload of an existing function is still a new function.)
  • Your method for determining the number of enumerators is broken. (It only works for trivial enums, and it adds a new enumerator, which is not ideal (unless there’s another reason for it?).)
  • Your method for mapping enumerators to their strings is broken. (You assume a trivial enum, where you can cast the enumerator value to an index.)
  • Your method for parsing enumerator strings will only work properly for trivial enums. (For non-trivial enums it will return weird results, but technically (so far as I can tell) won’t actually trigger UB.)

I know your primary performance concern is the compile time, but just in case you do care about run time performance… this code is extremely inefficient for what it actually does. It sounds like you don’t care; you figure it will all be constexpred away. Well, maybe, maybe not. There doesn’t seem to be any real reason not to make it more efficient in either case.

Style-wise, my biggest complaint would have to be the complete lack of any commenting or documentation. I know commenting a macro isn’t the easiest thing to do. But it’s still worth it; especially for a macro as complicated as this.

It also doesn’t help that most of your identifiers are terrible. jbegin? What’s “j”? ArrStrTy? Whut? A struct that contains only the members begin and len being called BeginLen? That’s really not helpful at all. A function called to_string() that doesn’t actually convert to a string?

The other thing that raises my hackles is that your macros are mixed-case. That’s just BEGGING for trouble. (Even more alarming is that you use mixed-case macros WHILE ALSO USING MIXED-CASE IDENTIFIERS!!! There’s a point where you’ve loaded a gun, pointed it at your foot, taken the safety off, and started feathering the trigger, and if you end up blowing your foot off, no one can possibly take you seriously for being surprised.)

(I’d also raise complaints about the header file being named “enum.h”. “.h” is for C header files. For C++ header files, you should use “.hpp”, “.hxx”, “.hh”, or something similar.)

Overall design-wise… I might be convinced of the utility of a make-an-enum-with-a-built-in-to_string macro. But not if that macro is as complex as this. A macro should make my code easier… if a macro makes things harder (to understand, to debug), then it’s not worth it. If you could make this macro much, MUCH simpler (I have to figure out an ad hoc state machine just to get enumerator strings?!), then it might be worthwhile.

\$\endgroup\$
1
  • \$\begingroup\$ You make good points. I’ll accept if there are no other answers. I am still hoping to get feedback on constexpr-ness and compilation speed especially compared to other enum macros. FWIW I agree that using a regular enum is the best option but macros like this are prevalent and can be a bottle neck in compilation (according to clang’s trace at least). \$\endgroup\$ Jun 12, 2020 at 18:34

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.