Should enum_name(E value) (and maybe others?) be prevented from working with empty enums? about magic_enum HOT 9 CLOSED

For full disclosure - my current guess for the root cause was that the refactoring accidentally introduced an ODR violation. The code that was pulled out was missing the include for the enum, so enum_name(E value) was treating it as an empty enum in that TU. This conflicts with the definition in the symptomatic TU, and the linker (?) ends up pulling in the empty-enum definition into the symptomatic TU.

Having enum_name reject empty enums would have helped catch this much earlier, and would also have pointed me to the problematic TU, which would have made debugging this much faster.

from magic_enum.

Hi,

1. I think that adding asserts is the easiest option without breaking backward compatibility (like return std::optional<std::string_view>)

2. I’m also thinking of adding settings via macros(?) to disable these asserts.

3. I think it's worth adding this to all APIs.

What do you think about it?

from magic_enum.

from magic_enum.

I am doing something similar on purpose:

    // Forward declare (values are intentionally deferred until after the
    // names TESTs are defined):
    enum MyModuleTapNames: uint32_t;

    // later, in the .cpp file:
    enum MyModuleTapNames {
        Foo,
        Bar,
    };

This allows me to hide the definitions from all translation units except the one that cares about it, and erase type information to generically have a system to grab enum names and counts from each translation unit.

should enum_name(E value) work at all if the enum is empty

We should distinguish here between enum MyModuleTapNames {}; and enum MyModuleTapNames;; the latter type is still incomplete and I think it should be rejected. (Or, provide an API to check if it is complete - which is how I found this issue :) But the former type has been completely defined to have no entries and I think it should be accepted.

If there's some technical restriction that prevents distinguishing the two cases, then let me know.

from magic_enum.

If there's some technical restriction that prevents distinguishing the two cases, then let me know.

At least based on some cursory searches it seems a fully conforming generally-useable is_complete template is not possible to implement. The tricky part is that template instantiations using the same type(s) need to behave the same, otherwise you (can?) get ODR violations with unpredictable results (like the behavior that spawned this issue :P). In addition, the behavior of such a thing may be counterintuitive if you have is_complete<T> followed by a definition of T followed by another is_complete<T>. You may be able to get around this by limiting instantiations to a single TU and/or by using __COUNTER__, but the latter isn't technically conformant and I'm not confident the former is guaranteed to work, especially if present in a header file.

That's part of why I suggested an is_empty flag - to allow magic_enum to distinguish between "this enum is intentionally empty" and "this enum may be empty or forward-declared and I can't tell". Of course, if there is a way to detect a forward-declared enum that solution seems moot.

from magic_enum.

@ts826848 @thirtythreeforty Please check this PR for possible solution #323

from magic_enum.

This partially works for me. It does correctly reject a declaration like

enum class MyEnum;

But, it does not reject a declaration that includes the underlying type:

enum class MyEnum: uint32_t;

from magic_enum.

Please check this PR for possible solution #323

The PR catches the error that I originally ran into, so I think it works for me. I think it should work for most others users as well; the only exception would be if a user intentionally has both empty and non-empty enums but prefers to keep the checks for non-empty enums if possible. I don't know whether that case will come up at all, though, and I think allowing checks to be done on a case-by-case basis should be fairly straightforwards.

I've left a few comments on the PR as well, but they're pretty much nitpicks.

But, it does not reject a declaration that includes the underlying type:

That's strange. The check is just std::is_enum_v<E> & (count_v<E, S> > 0), so it's not immediately obvious to me why the presence/absence of a declared underlying type should matter, especially for a scoped enum. I can't reproduce this behavior using either Clang or GCC as well. My test case, run in a file in the repo root:

#include "include/magic_enum/magic_enum.hpp"

enum class Forward : uint32_t;
auto f(Forward f) { return magic_enum::enum_name(f); }

I get a static_assert whether Forward has a declared type or not.

from magic_enum.

Ah, sorry. You are correct. I was relying on CLion's static analysis rather than running a build. I see your described behavior on Clang 16 and GCC 13.2. So it is working for me! Thank you!

from magic_enum.