Summary: | Use enum classes within FileSystem | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Don Olmstead <don.olmstead> | ||||||||||||||
Component: | Platform | Assignee: | Don Olmstead <don.olmstead> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, buildbot, chris.reid, commit-queue, darin, don.olmstead, mmaxfield, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Don Olmstead
2017-08-03 19:00:33 PDT
Created attachment 317198 [details]
Patch
Moves over to enum classes. Let me know if I should use a fixed type for the enum.
Well at least it worked on windows Created attachment 326129 [details]
Patch
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.
Created attachment 326130 [details]
Patch
Created attachment 326133 [details]
Patch
Removing what seems to be unnecessary explicit templates
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.
Comment on attachment 326133 [details] Patch Clearing flags on attachment: 326133 Committed r224505: <https://trac.webkit.org/changeset/224505> All reviewed patches have been landed. Closing bug. 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. 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". 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. Reopening for cleanup Created attachment 326412 [details]
Patch - Additional Cleanup
Reuploading the last patch so it can get queued in to EWS.
Comment on attachment 326412 [details] Patch - Additional Cleanup Clearing flags on attachment: 326412 Committed r224635: <https://trac.webkit.org/changeset/224635> All reviewed patches have been landed. Closing bug. |