Bug 142429 – Merge TrivialAtom and CharacterSet into a Term abstraction, prepare Term for composition

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Merge TrivialAtom and CharacterSet into a Term abstraction, prepare Term for composition

[Benjamin Poulain]

Created attachment 248139 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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?

[Alex Christensen]

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?

[Benjamin Poulain]

Committed r181282: <http://trac.webkit.org/changeset/181282>

[Benjamin Poulain]

(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.

[Benjamin Poulain]

(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.