Bug 175172 – Use enum classes within FileSystem

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Don Olmstead]

Turn FileOpenMode, FileSeekOrigin, and FileLock mode enums into enum classes.

[Don Olmstead]

Created attachment 317198 [details]
Patch

Moves over to enum classes. Let me know if I should use a fixed type for the enum.

[Don Olmstead]

Well at least it worked on windows

[Christopher Reid]

Created attachment 326129 [details]
Patch

[Build Bot]

Attachment 326129 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/EnumTraits.h:63:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Christopher Reid]

Created attachment 326130 [details]
Patch

[Christopher Reid]

Created attachment 326133 [details]
Patch

Removing what seems to be unnecessary explicit templates

[Christopher Reid]

Comment on attachment 326133 [details]
Patch

r?

One concern is that WebCore::FileSystem::FileOpenMode::<Enum> is a bit namespace heavy.
I added 'using namespace WebCore::FileSystem' where this namespace chain occurs multiple times.
Let me know if it is preferable to move these enum classes out of the filesystem namespace or even in to its own file.

[WebKit Commit Bot]

Comment on attachment 326133 [details]
Patch

Clearing flags on attachment: 326133

Committed r224505: <https://trac.webkit.org/changeset/224505>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 326133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326133&action=review

> Source/WebCore/platform/FileSystem.h:77
> +enum class FileOpenMode {

When changing to an enum class we should consider shortening names, since the enum name is always a qualifier. For example, these names should just be Read/Write/EventsOnly or possibly ForRead/ForWrite/ForEventsOnly if that seems much clearer. There is no need for the OpenFor prefix when FileOpenMode:: is already a prefix.

Also no need for the "= 0".

> Source/WebCore/platform/FileSystem.h:85
> +enum class FileSeekOrigin {

Same here: Beginning/Current/End.

Also no need for the "= 0".

> Source/WebCore/platform/FileSystem.h:91
> +enum class FileLockMode {

Same here: Shared, Exclusive, Nonblocking.

But an enum class is not a good fit for masks that need to be or-ed together. In modern WebKit code this should be an OptionSet instead.

[Darin Adler]

Comment on attachment 326133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326133&action=review

>> Source/WebCore/platform/FileSystem.h:91
>> +enum class FileLockMode {
> 
> Same here: Shared, Exclusive, Nonblocking.
> 
> But an enum class is not a good fit for masks that need to be or-ed together. In modern WebKit code this should be an OptionSet instead.

Note that "nonblocking" is a single word so it’s incorrect to capitalize the "B".

[Christopher Reid]

Created attachment 326406 [details]
Patch - Additional Cleanup

Thanks for the feedback Darin. Here's a patch addressing your suggestions.

> But an enum class is not a good fit for masks that need to be or-ed together. In modern WebKit code this should be an OptionSet instead.

Ah, I didn't realize WebKit had that. I have updated uses of the FileLockMode enum class to use an OptionSet instead.

[Don Olmstead]

Reopening for cleanup

[Christopher Reid]

Created attachment 326412 [details]
Patch - Additional Cleanup

Reuploading the last patch so it can get queued in to EWS.

[WebKit Commit Bot]

Comment on attachment 326412 [details]
Patch - Additional Cleanup

Clearing flags on attachment: 326412

Committed r224635: <https://trac.webkit.org/changeset/224635>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35562251>