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

Scope enum StructureState #566

Merged
merged 6 commits into from
Aug 3, 2020
Merged

Scope enum StructureState #566

merged 6 commits into from
Aug 3, 2020

Conversation

Brett208
Copy link
Member

@Brett208 Brett208 commented Aug 3, 2020

A couple of interesting changes were required to prevent some casting. I think it all improves readability.

This changeset will likely cause merge conflict in other uncommitted branches due to the prolific use of StructureState.

I wasn't sure it was best to move StructureState out of Structure.h, but I didn't think it was wise for Structure.h to reference Common.h and Common.h to reference Structure.h at the same time. And I'm not sure you are allowed to forward declare enum class? If this move isn't palatable, maybe there is an alternate solution.

Copy link
Member

@DanRStevens DanRStevens left a comment

Choose a reason for hiding this comment

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

I like the function signature changes to avoid casting. That was a good call.

Cyclic dependencies are probably best avoided with the header files. Assuming they have #pragma once, or header guards, it shouldn't matter, but still best to avoid it. With that said, I do kind of want to move away from dumping lots of different stuff in Common.h. I figure Structure related enum definitions are probably better suited to being part of the Structure class. Perhaps something we can look into at a later time.

I believe enum class can be forward declared. In particular, an enum class always has a base type, either implied, or explicitly specified. That means the compiler will always know how much space to allocate for it. That wasn't the case for regular enum definitions, when a base type wasn't specified, as the size could depend on the defined values.

@Brett208
Copy link
Member Author

Brett208 commented Aug 3, 2020

Ah right. I had mixed up which could be forward declared. I'll try to forward declare before merging.

Forward declare StructureState enum within Common.h
@Brett208
Copy link
Member Author

Brett208 commented Aug 3, 2020

Apparently you cannot forward declare an enum class that is the member of a class...

@Brett208 Brett208 merged commit 5f38c18 into master Aug 3, 2020
@Brett208
Copy link
Member Author

Brett208 commented Aug 3, 2020

Tagging #319

@DanRStevens DanRStevens deleted the ScopeEnumStructureState branch August 3, 2020 23:06
@DanRStevens
Copy link
Member

Huh, Interesting. Seems the scope nesting does pose problems for enum forward declarations. I had to try it just to see what happens. And of course, check StackOverflow 😁
Is in-class enum forward declaration possible?

Apparently it really isn't possible.

Anyway, looks like you found a pretty good solution.

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.

2 participants