[Webkit-unassigned] [Bug 52983] Eliminate m_tagHistory pointer from CSSSelector
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jan 23 16:20:27 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=52983
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #79876|review? |review+
Flag| |
--- Comment #2 from Darin Adler <darin at apple.com> 2011-01-23 16:20:27 PST ---
(From update of attachment 79876)
View in context: https://bugs.webkit.org/attachment.cgi?id=79876&action=review
For some reason the EWS bot can’t apply this patch. You may want to find out why, because it can be useful to know if any builds are broken.
Basic technique seems good; it’s unfortunate the details get a bit messy. More smart pointer use may help a little even though we’ll need to move things in and out of the smart pointers with adopt and leak.
I think that CSSParserSelector would be better if it had functions for all the things that the grammar wants to do to a selector. There’s no reason the grammar needs to reach through it by calling selector() each time. A few inline functions for the operations done by the grammar wouldn’t be all that hard to write, and generally speaking we want to keep the grammar file as simple as possible and put as much of the complexity in code outside that file.
I’m going to say review+ because after reading all my comments none seems mandatory, but I think there is room for improvement here, and I’d really like to see EWS results.
> Source/WebCore/css/CSSGrammar.y:1218
> + CSSParserSelector* parserSelector = p->sinkFloatingSelector($4);
> + CSSSelector* simpleSelector = parserSelector->releaseSelector();
> + $$->selector()->setSimpleSelector(simpleSelector);
> + delete parserSelector;
The sinkFloatingSelector function should return a PassOwnPtr<CSSParserSelector>. Similarly, the releaseSelector function should return a PassOwnPtr<CSSSelector> and the setSimpleSelctor function should take a PassOwnPtr. Then we would not need any local variables here. It would just read:
$$->selector()->setSimpleSelector(p->sinkFloatingSelector($4)->releaseSelector());
And all the ownership transfer would be taken care of.
> Source/WebCore/css/CSSParser.cpp:5706
> -CSSSelector* CSSParser::sinkFloatingSelector(CSSSelector* selector)
> +CSSParserSelector* CSSParser::sinkFloatingSelector(CSSParserSelector* selector)
This should return a PassOwnPtr as I mentioned above.
> Source/WebCore/css/CSSParserValues.cpp:80
> + , m_tagHistory(0)
I find the name “tag history” confusing. But it’s not like it’s a new term, so not your problem.
> Source/WebCore/css/CSSParserValues.h:98
> + CSSSelector* releaseSelector() { return m_selector.leakPtr(); }
This should return a PassOwnPtr.
> Source/WebCore/css/CSSParserValues.h:100
> + void setTagHistory(CSSParserSelector* selector) { m_tagHistory = selector; }
This should take a PassOwnPtr.
> Source/WebCore/css/CSSParserValues.h:104
> + CSSParserSelector* m_tagHistory;
This should be an OwnPtr too. You can call leakPtr on it in the destructor.
> Source/WebCore/css/CSSSelector.h:256
> + // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
> + const AtomicString& value() const { return *reinterpret_cast<const AtomicString*>(m_hasRareData ? &m_data.m_rareData->m_value : &m_data.m_value); }
I wonder if there’s a way to avoid this. I believe we’ve had trouble with this kind of code in the past, with high optimization settings in the compiler. Maybe we can have value() just return an AtomicStringImpl*?
> Source/WebCore/css/CSSSelector.h:272
> + void setValue(const AtomicString& value)
> + {
> + if (m_hasRareData) {
> + m_data.m_rareData->m_value = value.impl();
> + m_data.m_rareData->m_value->ref();
> + return;
> + }
> + // Need to do ref counting manually in the union.
> + m_data.m_value = value.impl();
> + m_data.m_value->ref();
> + }
When a function gets this long I prefer to put the definition outside the class. Maybe we should take a pass through this class and move the function bodies out of the class. I find it makes the class way easier to read.
> Source/WebCore/css/CSSSelector.h:329
> + RareData(AtomicStringImpl* value)
> + : m_value(value)
> + , m_a(0)
> , m_b(0)
> - , m_tagHistory(tagHistory)
> , m_attribute(anyQName())
> , m_argument(nullAtom)
> {
> }
> + ~RareData()
> + {
> + if (m_value)
> + m_value->deref();
> + }
This seems hard to use correctly. The constructor doesn’t ref but the destructor does deref. I have no doubt the code currently handles this correctly, but I suspect it will be easy to screw this up later.
One way to do this is to make the constructor argument be a PassRefPtr<AtomicStringImpl> and then call leakPtr on it. This makes it much harder to use it incorrectly.
> Source/WebCore/css/CSSSelector.h:334
> + AtomicStringImpl* m_value;
Why not use AtomicString or RefPtr<AtomicStringImpl> here? I don’t think a raw pointer is best. It’s easy to transfer ownership from a raw pointer, by calling adoptRef on it. Even though you need to use a raw pointer in the union, I think it’s better to not use a raw pointer here as well.
> Source/WebCore/css/CSSSelector.h:353
> - DataUnion() : m_tagHistory(0) { }
> - CSSSelector* m_tagHistory;
> + DataUnion() : m_value(0) { }
> RareData* m_rareData;
> + AtomicStringImpl* m_value;
The old union used to initialize the first value. Now you’re initializing the second, which is a little stranger. Obviously the order doesn’t matter, but it kind of attracts the attention a tiny bit.
> Source/WebCore/css/CSSSelectorList.cpp:50
> + const size_t vectorSize = selectorVector.size();
> + size_t arraySize = 0;
I don’t think it’s all that helpful to distinguish the passed-in vector from the adopted one by calling the latter an array. I know it’s using malloc directly, but that seems an implementation detail. I think the internal one could be called something like “flattened” instead. Or “complete”. Or we could call it the “list” since it’s the actual list.
> Source/WebCore/css/CSSSelectorList.cpp:60
> + delete selectorVector[0];
This makes it clear that selectorVector should be a Vector<OwnPtr<CSSParserSelector> >.
Doing the deletion by hand is trickier than it needs to be.
> Source/WebCore/css/CSSSelectorList.cpp:64
> + m_selectorArray = reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * arraySize));
This should be static_cast, not reinterpret_cast.
> Source/WebCore/css/CSSSelectorList.cpp:70
> + memcpy(&m_selectorArray[arrayIndex], selector, sizeof(CSSSelector));
It’s dangerous to move a CSSSelector this way unless the class knows about it; some classes have pointers into themselves or other such things. If we really need this move operation, we should make a function member of the CSSSelector class to do the work. That makes it more likely that someone changing CSSSelector around won’t invalidate this operation. We can just name the function move or moveIntoUninitializedMemory or something like that and make it either a member function or static member function.
> Source/WebCore/css/CSSSelectorList.h:49
> + static CSSSelector* next(CSSSelector* current) {
> + while (!current->isLastInTagHistory())
> + current++;
> + return current->isLastInSelectorList() ? 0 : current + 1;
> + }
Same comment about keeping long functions out of the class. I also think that the various places where we do the “+ 1” trick to find adjacent elements need “why” comments.
> Source/WebCore/css/CSSSelectorList.h:50
> + bool hasOneSelector() const { return m_selectorArray ? !next(m_selectorArray) : false; }
I prefer && over ? : for cases like this, even thought the old code used ? :
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list