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

Fix plenty of clang warnings declaration shadows a variable: using class enum instead of enum #815

Closed

Conversation

hyperxor
Copy link
Contributor

@hyperxor hyperxor commented Feb 8, 2020

I have got a lot warnings from clang compiler:

yaml-cpp/src/emitterstate.h:28:24: warning: declaration shadows a variable in namespace 'YAML' [-Wshadow]
  enum value { NoType, Flow, Block };
                       ^

and my proposal is to use enum classes.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
… enum
@hyperxor hyperxor changed the title Fix clang declaration shadows a variable: using class enum instead of enum Fix clang warning declaration shadows a variable using class enum instead of enum Feb 8, 2020
@hyperxor hyperxor changed the title Fix clang warning declaration shadows a variable using class enum instead of enum Fix plenty of clang warnings declaration shadows a variable: using class enum instead of enum Feb 9, 2020
Copy link
Owner

@jbeder jbeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably right, but it's a breaking change (e.g. see https://stackoverflow.com/questions/10361507/link-compatibility-of-enums-and-enum-classes), so we'll bump the version to 0.7.0 next.

Can you make sure you've run clang-format on the changed lines?

@hyperxor
Copy link
Contributor Author

@jbeder Hello, unfortunatelly, this change breaks API, and shouldn't be done in minor version.
I run clang-format and push the following changes. In general, I think this change makes API clear and better, but I'm not hurry and it can be merged to master later.

@rickyjames35
Copy link

I see this got closed by never made it into a release. Any chance we can get this in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants