Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of all-caps macro-style names for enum members collides with macros #644

Closed
ogrant opened this issue Feb 8, 2018 · 7 comments
Closed

Use of all-caps macro-style names for enum members collides with macros #644

ogrant opened this issue Feb 8, 2018 · 7 comments

Comments

@ogrant
Copy link

@ogrant ogrant commented Feb 8, 2018

Hi,

The Type enum defined in format.h uses all uppercase names for its members which easily collides with macro names, especially as those names are commonly used by C libraries:

  enum Type {
    NONE, NAMED_ARG,
    // Integer types should go first,
    INT, UINT, LONG_LONG, ULONG_LONG, BOOL, CHAR, LAST_INTEGER_TYPE = CHAR,
    // followed by floating-point types.
    DOUBLE, LONG_DOUBLE, LAST_NUMERIC_TYPE = LONG_DOUBLE,
    CSTRING, STRING, WSTRING, POINTER, CUSTOM
  };

Great library btw.

Thanks,

O.

@vitaut

This comment has been minimized.

Copy link
Contributor

@vitaut vitaut commented Feb 10, 2018

Did you have a collision with any specific name? Normally it's easy to fix this by ordering includes but I'm open to changing some names if there are collisions with a widely used C library.

@t-b

This comment has been minimized.

Copy link

@t-b t-b commented Feb 10, 2018

@vitaut Are these constants for users? If not I'd rather either convert them to enum class if you are targeting C++11 or put them into a detail namespace.

@vitaut

This comment has been minimized.

Copy link
Contributor

@vitaut vitaut commented Feb 11, 2018

@t-b They are already in the internal namespace, but the question here is collision with global macros.

@LarsGullik

This comment has been minimized.

Copy link

@LarsGullik LarsGullik commented Feb 16, 2018

I see collisions as well, up until now I have modified locally the names to have a FMT_ prefix.
In particular LONG_LONG has been a culprit. (I sent a patch that fixes it in the way that we have done internally, you might want to do something else.)

@ogrant

This comment has been minimized.

Copy link
Author

@ogrant ogrant commented Feb 16, 2018

Sorry for not getting back to you earlier.

I think in this particular case, it was ruby.h, which is probably not very popular.

But independently from that, I would humbly argue that giving C++ enums all caps names, especially one as common as LONG_LONG which is used in many C libs is just asking for trouble. I would suggest moving to a more C++ naming convension (eName or kName), or basically anything else than all caps.

vitaut added a commit that referenced this issue Feb 16, 2018
@vitaut

This comment has been minimized.

Copy link
Contributor

@vitaut vitaut commented Feb 16, 2018

Fixed in 8ed264f.

@vitaut vitaut closed this Feb 16, 2018
@ogrant

This comment has been minimized.

Copy link
Author

@ogrant ogrant commented Feb 16, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.