Replace all enum to enum class. Rename some enum members where appropriate (for example: (RoleType)NameRole -> RoleType::Name).
Diff Detail
- Repository
- R318 Dolphin
- Branch
- enum-class
- Lint
No Linters Available - Unit
No Unit Test Coverage
Thanks for the patch.
I agree we should use enum class for new code, I'm not sure it's worth porting to old enums though.
The problem is that this diff is huge and it will make git blame harder to use, for little gain.
@markg @rkflx toughts?
That said, if we really want to do it, please get rid of those static_casts where possible, they will only reduce the readability of the code.
src/dolphincontextmenu.cpp | ||
---|---|---|
69 | If we change the type of m_context, we don't need the static_cast. |
Tricky one!
On one side i'm all in for code improvements and modernization.
On the other hand, only if it makes sense.
As it's done now (with all the added static_casts's) it's imho not worth it as the new code looks less clean then the old one, but that can be fixed.
I personally would not bother making a patch for it, but now that the patch is there. Why not :)
New code should certainly use enum class!
Lastly, please do get the permission of the Dolphin maintainer as this is quite a refactoring.
Late to the party, here are my thoughts:
There's some value in the explicitness enum class sometimes provides, but counteracting it immediately with static_cast is not really useful.
I'd vote to apply this to new code only for now. Speaking from recent experience, it's easy to get things wrong regarding an enum refactoring ;) Better not to perform a mass change, unless there is a reviewer checking very thoroughly (I won't).
Another variant is to apply only obvious changes and skip all changes that require static_casts. It's not hard to do (I need just filter my changes), easy to check for a reviewer (verify that (X)SomethingPostfix -> X::Something) and better than nothing.
What about this diff? Will this be accepted if I revert all enums that require static_cast?
Large review!
I was going to give a ship it till i compile tested it. It failed!
You can find the output here: https://p.sc2.nl/HJDBOJmtM
There is no reason to rush this in now, I'd suggest to wait the feature freeze on March 22 [1] so that we have enough time to be sure we don't break anything.
[1]: https://community.kde.org/Schedules/Applications/18.04_Release_Schedule
It's strange. It builds even without additional warnings: https://gist.github.com/rominf/d3d55d85c23f83366a1ae7b5af525613