[Webkit-unassigned] [Bug 82577] Work around an entity parsing bug in libxml2 2.7.3 (supplied with Lion) and unskip tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=82577





--- Comment #9 from Filip Pizlo <fpizlo at apple.com>  2012-04-01 11:54:13 PST ---
(In reply to comment #6)
> (From update of attachment 134993 [details])
> Can you create a USE(LIBXML_ENTITY_PARSER_WORKAROUND) ifdef and put all this code behind the ifdef?  That will make it easier for us to remove it in the future.

I think that this patch makes removing the hack pretty easy already, since the things it does are guarded by hackAroundLibXMLEntityParsingBug().  Furthermore, the behavior of hackAroundLibXMLEntityParsingBug() already depends on the libxml version, and already has a comment describing that it is a workaround for a bug that existed before 2.7.4.

I also think it's good for code quality to have this patch use a guard that can be checked using a C++ conditional rather than a C preprocessor conditional, since the latter tends to make the code ugly. Though in this patch, it may not make a big difference, and if it were me, I'd probably have used C preprocessor conditionals, for the reasons that Eric stated.

The thing that C preprocessor conditionals instead of C++ conditionals would give us here is to remove the 8 bytes of data that seem to be added to the XMLDocumentParser class. But that does not seem like much, since the class already has a lot of state. In particular, most of the other cruft that this code adds ought to be removed at compile-time already. hackAroundLibXMLEntityParsingBug() is a static inline that returns a constant, so that should result in simplification of control flow.  entityDeclarationHandler() is the only source of time overhead; at best it will be a tail call, leading to a useless jump. At worst, it'll be a handful of instructions that do some stack fiddling. But I can't imagine that matters much.

Nikolas, do you have insight into what the performance and memory usage impacts of this patch are?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.