Polish nsAttrAndChildArray::GetSafeAttrNameAt




13 years ago
13 years ago


(Reporter: sicking, Assigned: sicking)



Firefox Tracking Flags

(Not tracked)



(1 attachment)

Created attachment 207220 [details] [diff] [review]
possible patch

I'm not very convinced that this is a good idea since the rest of the code checks for the existance of an attribute the way that GetSafeAttrNameAt currently does it. So right now it's consistent with the rest of the code.

AttrNameAt doesn't bother with checking for existance of attributes, which is why the code there looks different.
Attachment #207220 - Flags: superreview?(bzbarsky)
Attachment #207220 - Flags: review?(bzbarsky)
Under what conditions would we be missing an attribute here anyway?  That is, why do we need this check?
When an attribute is removed we don't move all the children in the buffer, but rather leave a gap between the last attribute and the first child. But we only keep track of how many attribute slots are allocated, not how many are actually used.
In that case, the current impl of nsAttrAndChildArray::AttrNameAt is wrong, no?  It'll sometimes return null and miss out on real attributes...
No, we compress the attributes part of the buffer so there are never any empty slots in the middle of the array, just between the attributes and the children.

(Note that AttrNameAt will never return null, even on bad accesses to out-of-bounds indexes. It'll return a pointer to a null value)
I guess I'm trying to understand what comment 0 is really saying.  As long as we start walking at ATTRS() and walk by steps of 1 for AttrCount() steps, we'll hit all our attributes and nothing else, right?  Without a need to check what's in that slot as GetSafeAttrNameAt currently does?
Well, AttrSlots() is implemented by walking the buffer and checking for the first non-empty slots, so we'd check for empty slots no matter what. However doing it the way that GetSafeAttrNameAt is faster since we'll only check a single slot (the one requested)
(In reply to comment #7)
> Well, AttrSlots() is implemented by walking the buffer and checking for the
> first non-empty slots

What's AttrSlots()?  The code in AttrNameAt uses ATTRS(mImpl).  Is that really slower than what GetSafeAttrNameAt does?

I think I'm starting to see what you meant in general, though.  The problem is that we don't store the number of attrs we have, just the number of slots.  And some number of slots at the end may hold null, but for purposes of GetSafeAttrNameAt we don't care how many.  OK.  Given that, I agree that this change is kinda silly.

Let me know whether you still want review or whether you want to just wontfix?
Sorry, i meant AttrCount().

Yeah, it's the fact that we don't store the real attrcount that calls for the current implementation. Would have been easier to do this over irc, it's good to have you back from vacation :)

Using ATTRS more in general would be a good thing I agree. It'd probably not make a difference for the compiled code, but is easier to read. But I'd want to do it consistently everywhere where we check for empty slots and not just in GetSafeAttrNameAt.

Next time I'm in this class I'll make an effort clean things up (with an eye towards how I did this in nsMappedAttributes).
Last Resolved: 13 years ago
Resolution: --- → WONTFIX
Attachment #207220 - Flags: superreview?(bzbarsky)
Attachment #207220 - Flags: review?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.