Merge TrivialAtom and CharacterSet into a Term abstraction, prepare Term for composition
Created attachment 248139 [details] Patch
Attachment 248139 [details] did not pass style-queue: ERROR: Source/WebCore/contentextensions/URLFilterParser.cpp:487: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 248139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248139&action=review > Source/WebCore/contentextensions/URLFilterParser.cpp:51 > + enum class TermType : uint8_t { > + Empty, > + Deleted, > + CharacterSet > + }; This private enum can be in the private: section. There’s no need to have it at the top of the class. > Source/WebCore/contentextensions/URLFilterParser.cpp:55 > + Term() > + { > + } Why are all the function bodies in the class? To me this seems to make the class harder to read. > Source/WebCore/contentextensions/URLFilterParser.cpp:281 > + ~AtomData() > + { > + } Is this explicit definition of the destructor needed? If so, why?
Comment on attachment 248139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248139&action=review > Source/WebCore/contentextensions/URLFilterParser.cpp:261 > + BitVector characters { 128 }; If I understand this correctly, the whole point of using a union, placement new, and explicitly calling destructors is to avoid allocating and deallocating a BitVector for empty/deleted slots in a hash table. Won't this all be excessive if we switch to using two uint64_t's?
Committed r181282: <http://trac.webkit.org/changeset/181282>
(In reply to comment #4) > Comment on attachment 248139 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248139&action=review > > > Source/WebCore/contentextensions/URLFilterParser.cpp:261 > > + BitVector characters { 128 }; > > If I understand this correctly, the whole point of using a union, placement > new, and explicitly calling destructors is to avoid allocating and > deallocating a BitVector for empty/deleted slots in a hash table. Won't > this all be excessive if we switch to using two uint64_t's? The union is there to support other terms without rewriting everything.
(In reply to comment #3) > Comment on attachment 248139 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248139&action=review > > > Source/WebCore/contentextensions/URLFilterParser.cpp:51 > > + enum class TermType : uint8_t { > > + Empty, > > + Deleted, > > + CharacterSet > > + }; > > This private enum can be in the private: section. There’s no need to have it > at the top of the class. IIRC, some compiler complained about it when it was at the end of the file. Clang seems fine with it, let's try again. > > Source/WebCore/contentextensions/URLFilterParser.cpp:55 > > + Term() > > + { > > + } > > Why are all the function bodies in the class? To me this seems to make the > class harder to read. JavaScriptCore users that style for private generators so I copied that here. > > Source/WebCore/contentextensions/URLFilterParser.cpp:281 > > + ~AtomData() > > + { > > + } > > Is this explicit definition of the destructor needed? If so, why? It is. The union has non trivial destructor so they need to be invoked explicitly. In this case, the destructor ignore the cleanup and the destroy() function takes care of it.