MSVC Code Analysis warnings suggesting enum class

[DanRStevens]

Running MSVC's Code Analysis (Analyze -> Run Code Analysis -> On Solution) gives many warnings, most of which suggest changing enum to enum class.

Warnings filtered for enum class suggestions:

C:\Users\User\source\repos\vcpkg\installed\x64-windows\include\SDL2\SDL_rect.h(102): warning C26812: The enum type 'SDL_bool' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\nas2d-core\include\NAS2D\Signal.h(65): warning C26812: The enum type 'DiggerDirection::DiggerSelection' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Structures\Structure.h(68): warning C26812: The enum type 'Structure::StructureState' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Structures\Structure.h(72): warning C26812: The enum type 'DisabledReason' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Structures\Structure.h(79): warning C26812: The enum type 'IdleReason' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Structures\Structure.h(103): warning C26812: The enum type 'Structure::StructureClass' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Structures\Structure.h(104): warning C26812: The enum type 'ConnectorDir' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Common.cpp(82): warning C26812: The enum type 'MineProductionRate' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Common.cpp(232): warning C26812: The enum type 'ProductType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Mine.h(37): warning C26812: The enum type 'MineProductionRate' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\GraphWalker.cpp(13): warning C26812: The enum type 'Direction' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Structures\Factory.h(44): warning C26812: The enum type 'ProductType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Structures\SeedSmelter.h(53): warning C26812: The enum type 'ResourcePool::ResourceType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Things\Robots\Robodigger.h(13): warning C26812: The enum type 'Direction' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Map\TileMap.cpp(41): warning C26812: The enum type 'constants::PlanetHostility' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Map\TileMap.cpp(395): warning C26812: The enum type 'TileMap::MouseMapRegion' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Mine.cpp(14): warning C26812: The enum type 'Mine::OreType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\PopulationPool.cpp(42): warning C26812: The enum type 'Population::PersonRole' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\Population\Population.cpp(83): warning C26812: The enum type 'Population::PersonRole' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\ProductPool.cpp(16): warning C26812: The enum type 'ProductType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\ResourcePool.cpp(111): warning C26812: The enum type 'ResourcePool::ResourceType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\RobotPool.cpp(63): warning C26812: The enum type 'RobotType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewState.h(220): warning C26812: The enum type 'InsertMode' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewState.h(221): warning C26812: The enum type 'StructureID' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewState.h(222): warning C26812: The enum type 'RobotType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewState.cpp(95): warning C26812: The enum type 'constants::PlanetHostility' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewState.cpp(134): warning C26812: The enum type 'MapViewState::PopulationLevel' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\Planet.h(36): warning C26812: The enum type 'Planet::PlanetType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MainMenuState.cpp(169): warning C26812: The enum type 'FileIo::FileOperation' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewStateHelper.cpp(320): warning C26812: The enum type 'StructureID' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewStateHelper.cpp(534): warning C26812: The enum type 'RobotType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewStateIO.cpp(362): warning C26812: The enum type 'StructureID' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewStateUi.cpp(446): warning C26812: The enum type 'RobotType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewStateUi.cpp(448): warning C26812: The enum type 'InsertMode' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewStateUi.cpp(456): warning C26812: The enum type 'DiggerDirection::DiggerSelection' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\MapViewStateUi.cpp(561): warning C26812: The enum type 'FileIo::FileOperation' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\States\PlanetSelectState.cpp(190): warning C26812: The enum type 'constants::PlanetHostility' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\states\SplashState.cpp(34): warning C26812: The enum type 'LogoState' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\StructureCatalogue.cpp(25): warning C26812: The enum type 'StructureID' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\StructureTranslator.cpp(17): warning C26812: The enum type 'StructureID' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\UI\Core\Button.h(58): warning C26812: The enum type 'Button::State' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\UI\Core\Button.h(59): warning C26812: The enum type 'Button::Type' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\UI\Core\Slider.cpp(58): warning C26812: The enum type 'Slider::SliderType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\UI\Core\TextField.h(79): warning C26812: The enum type 'TextField::BorderVisibility' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\ui\FileIo.cpp(109): warning C26812: The enum type 'FileIo::FileOperation' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\UI\MajorEventAnnouncement.cpp(44): warning C26812: The enum type 'MajorEventAnnouncement::AnnouncementType' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).
Z:\OPHD\src\UI\ResourceBreakdownPanel.cpp(25): warning C26812: The enum type 'ResourceTrend' is unscoped. Prefer 'enum class' over 'enum' (Enum.3).

---

As a side note, some of those paths have inconsistencies in the casing. In particular:

• "states" in "Z:\OPHD\src\states\SplashState.cpp"
• "io" in "Z:\OPHD\src\ui\FileIo.cpp"

[DanRStevens]

Some notes that may help with automated changes:

From master: Using grep, a list of enum names, which have not been declared as enum class can be obtained with:

grep -rhoP "^\s*enum (?!(class))\K\w+" OPHD > enumNames.txt

Flags:

• -r - recursive, searches OPHD/ and all subfolders
• -h - no filename, suppress printing of the filename
• -o - only matching, print only the matched portion of a line
• -P - Perl regexp, interpret pattern as Perl regexp

Regexp:

• ^ - start of the line, don't allow skipping to start matching part way into the line
• \s - whitespace
• * - zero or more of the previous pattern (whitespace)
• (?!(...)) - negative lookahead, don't match if this pattern follows
• \K - look behind, only match if the previous pattern held, but don't include previous pattern in matched output
• \w - word characters
• + - match one or more of the previous pattern (word characters, hence forming a word)

That will extract all names that follow an enum keyword (but excludes matches that start with enum class).

The result is redirected to a file for later processing.

---

From the fix branch: A list of prefixed enum values can be obtained with:

xargs --arg-file=enumNames.txt -I'{text}' grep -rho "\b{text}::\w*" OPHD | sort | uniq > enumValues.txt

Flags (xargs):

• --arg-file - newline (or space) separated list of inputs to process
• -I - replace string, string that will be replaced in the following command with the input line

The grep flags are the same as above.

Regexp:

• \b - word boundary, only match on a word boundary (don't match word suffixes)
• {text} - not part of regexp, but rather the replacement text from xargs (enum name)
• :: - literal characters to match (scope resolution operator)
• \w* - a word (0 or more word characters), the enum value name

Since many enum values are used more than once, they will be found more than once. Hence we must sort the output, and then remove duplicates with uniq (unique). This utility only removes adjacent duplicates, so the output needs to be sorted first. The results are then redirected to a file.

The output file will have a couple of extraneous lines from a comment that was matched. These can be manually removed:

• State::initialize
• State::update

---

More to follow later....

[cugone]

Or just use Visual Assist's Refactor > Rename... and it'll automatically pick up every actual non-comment use, optionally across all projects. It'll only pick up uses in comments if the option is selected.

[DanRStevens]

I found something that largely works, using the input files generated above:

xargs --arg-file=enumNames.txt -I'{enumName}' sh -c 'echo "{enumName}:"; grep -hoP "^{enumName}::\K\w*" enumValues.txt | xargs -I"{enumValueName}" find OPHD/ -type f -exec sed -i -e "s/\(^\t*[^\t].*[^({enumName}::)]\)\b{enumValueName}\b/\1{enumName}::{enumValueName}/g" {} \; && make && git add OPHD/ && git commit -m "Adding prefix for enum {enumName}"'

It will go through each enum name, and do a global search/replace of all enum values to add the prefix. It then does a test build, and it that succeeds, it creates a commit with an automatic commit message.

There was one snag with TileMapLevel::LEVEL_SURFACE. It seems there is a global std::string variable named LEVEL_SURFACE, so trying to global search/replace that string results in problems. If that entry is removed from the enumValues.txt file, the above command works.

One other slight issue, is a similar case happened in comments, where comment text was prefixed with an enum name. It doesn't break the code, so the compile step still succeeded, but the result looks ugly. Comments were edited by:

• StructureState::DISABLED.
• StructureState::IDLE
• NavigationPanel::PANEL_PRODUCTION
• NavigationPanel::PANEL_MINING
• StructureID::SID_NONE

These were found afterwards using:

git log -p | grep "+.*//.*::" | more

Diff lines containing additions, with a comment, and the scope resolution operator added after the comment.

[DanRStevens]

Bit of an update. I noticed a few entries were missing from the enum values list I had extracted earlier. This resulted in unprefixed values in the auto processed results. Additionally, I needed to adjust the text replacement command to account for multiple replacements in one line.

The enum values previously missing were:

• FileOperation::FILE_SAVE
• StructureClass::CLASS_RECYCLING
• PlanetHostility::HOSTILITY_NONE
• PlanetHostility::HOSTILITY_LOW
• PlanetHostility::HOSTILITY_MEDIUM
• PlanetHostility::HOSTILITY_HIGH
• PopulationLevel::POPULATION_SMALL
• PopulationLevel::POPULATION_LARGE

I manually added those to the file, and re-ran the processing command.

The updated processing command was:

xargs --arg-file=enumNames.txt -I'{enumName}' sh -c 'echo "{enumName}:"; grep -hoP "^{enumName}::\K\w*" enumValues.txt | xargs -I"{enumValueName}" find OPHD/ -type f -exec sed -i -e "s/\([^\t]\)\b{enumValueName}\b/\1{enumName}::{enumValueName}/g; s/{enumName}::{enumName}::{enumValueName}/{enumName}::{enumValueName}/g" {} \; && make && git add OPHD/ && git commit -m "Adding prefix for enum {enumName}"'

Additionally, the output was checked by temporarily switching all enum to enum class and compiling, and checking for a specific error message related to missing scope prefixes:

make 2>&1 | grep "no member named"

This results in PR #371.

---

Once the branch for #371 was made, I then rebased existing changes from PR #368 on top of it, forcing merge conflicts to resolve to the original code. This effectively cut down the bulk of the remaining code, as only the non-prefix-addition changes still remained. The result of this was pushed as branch rebaseFix319.

Git rebase command:

git rebase -Xtheirs autoPrefixAdd

Confusingly, -Xtheirs is used to resolve changes to the current branch when using git rebase, which is opposite from when git merge is used with another branch.

---

I also went and undid some of the accidental whitespace changes. I might not have caught all whitespace changes, but I did grep for new lines containing known accidental whitespace change patterns. The result is branch rebaseFix319RemoveWhitespaceChanges. The whitespace changes can be checked by comparing with rebaseFix319. Going forward, we should probably use the branch with the whitespace changes stripped.

To check the whitespace changes, I used the following searches:

git show | grep "+\s*if("
git show | grep "+\s*} else"

The "+" was to find new added lines in the diff, which after some whitespace, contained some known changed pattern.

[Brett208]

What is the status of this issue, am I ok to slowly moving towards scoped enums or is there still a push to try and do it as a single large commit?

[DanRStevens]

So far, enum name scope prefixes have been added to all enum value names (required for enum class).

Still to change is actually adding the class keyword to each enum definition. We also need to rename the enum values. Enum value names should be UpperCamelCase. Enum name prefixes (not the scope prefix, but common name prefixes added to the value name itself) should be dropped. The scope prefix effective takes the place of the name prefix.

During the previous attempt, it was discovered a lot of static_cast needed to be added to get things to compile. This should be avoided. We may want to look into refactoring to remove the need for such casts. It does not affect all enums equally. As such, it may be easier to do the remaining changes in a piecemeal fashion, perhaps first focusing on the easy cases that don't need static_cast or some kind of refactor.

We may need to discuss cases that would rely on heavy static_cast to come up with an alternate solution. I'm afraid I've lost context on that part though, so I don't remember what cases that entailed, or how we might go about refactoring things instead.

[ldicker83]

To be fair, the casting is ... well, it's hideous. It's what happens when I want to "just get something our the door". In many cases a table lookup is a better option and will require some refactoring to get it looking good and functioning properly.

Anyway, I'm definitely in favor of moving toward enum class's versus abusing enumerated values.

[DanRStevens]

Ahh, to add some clarity, switching from enum to enum class may require adding more static_cast conversions to keep the code compiling, due to the more strict type checks with enum class. In particular, an enum class value name will not auto convert to an integer, so you can't use an enum class value name in a table lookup without a static_cast. At least not unless you write a type conversion operator to explicitly allow conversion to an integer type.

For enums that are meant to be used as table lookup, we'd probably want to refactor to add a type conversion operator, or we might want to look into refactoring the code so it doesn't use enum value names in table lookups.

[Brett208]

I would be interested to see how the type conversion operator works out if appropriate for some of the enums.

[DanRStevens]

I found this article interesting:
Using enum classes as type-safe bitmasks

It's more about bit manipulation operators, rather than type conversion operators, but the idea should be similar. There is the use of a macro near the end, to shorten enabling bit operators for enum classes. Maybe we want to avoid that part. Its been a while since I read the article, so I don't quite remember how big of a deal it was. I don't think it was super important.

[Brett208]

I was looking to scope the enums ConnectorDir and StructureID. They are both used with the int meta argument on IconGrid::addItem.

mStructures.addItem(constants::WAREHOUSE, 9, StructureID::SID_WAREHOUSE);

mConnections.addItem(constants::AG_TUBE_INTERSECTION, 110, ConnectorDir::CONNECTOR_INTERSECTION);

I'm not sure if anyone has a good idea for how to store the metadata in a way that doesn't require static_cast into and out of int for the two enums. (Maybe some sort of redesign to IconGrid or redesign the use of the enums?)

If we don't want to change using int meta in IconGrid, we could wrap these two enums in namespaces and leave them as non-scoped enums. This removes each individual enum member from the global namespace but doesn't provide some of the typing checks of enum class. Sort of have of enum class.

example:

namespace ConnectorDir
{
    enum ConnectorDir
    {
	Intersection = 1,
	Right,
	Left,
	Vertical // Functions as an intersection
    };
}

[DanRStevens]

Ok, looks like you've identified one of the more problematic cases. Perhaps we should look at this code and see if there is a way to redesign it.

Wrapping it in a namespace would ensure uses of the value names have to be scope prefixed. Though I suspect it would do nothing to eliminate the MSVC warning.

Maybe in the future we won't need an enum for connection direction. Maybe we can use raw integers, which do table lookups for properties that are specific to direction. I suspect the code using this enum could use a bit of refactoring, though don't have a fleshed out idea of where we want to take it. It might be that connector direction could become a visual only thing, and the game internally only marks where tubes are, not what direction they are facing. The direction/connectivity could be calculated using surrounding tiles.

[DanRStevens]

It appears there are currently only 3 enum definitions left that haven't been converted to enum class

• enum ConnectorDir
• enum ProductType
• enum StructureID

They are somewhat large and extensively used though, so perhaps more difficult to update than previous ones.