#15 Declined
Repository
p5-xml-libxml
Branch
default
Repository
perl-XML-LibXML
Branch
default

New improved (and leak-free) hash deref for XML::LibXML::Element

Author
  1. Toby Inkster
Reviewers
Description

Fixed memory leak since pull request #14. A new test is included for the memory leak. AttributeHash.pm now complies better with the HACKING.txt style guide. XML::LibXML::AttributeHash now requires Scalar::Util, but that's been part of Perl core since 5.8.

Description for pull request #14 is copied and pasted below.

OK, here's an improved version.

Firstly, I've dropped the array deref, as I have no rationale for including it.

Secondly, MANIFEST and Changes are now updated; documentation is included in XML::LibXML::AttributeHash (which is where the bulk of the new implementation is).

The new implementation uses Tie::Hash and Hash::FieldHash, making the dereferenced hash fully usable for read-write access to the element's attributes. So you can get/set, can check "exists" on keys, iterate through them, etc.

The primary rationale for this is that when using a DOM in most other languages, attributes can be accessed in a manner similar to this. For example, in Javascript:

var links = document.getElementsByTagName('a'); window.alert(links[0].href);

The patch allows Perl code to function similarly:

my links = $document->getElementsByTagName('a'); warn $links[0]{'href'};

The one drawback is that the implementation requires Hash::FieldHash, thus adding an extra non-core dependency. However, Hash::FieldHash is very lightweight (an XS module with no non-core dependencies) and reliable (776 PASSes on CPAN testers, with just one FAIL).

Also included are a bunch of whitespace improvements for LibXML.pm (mostly removing tabs).

Comments (4)

  1. Shlomi Fish repo owner

    Hi Toby,

    I got the notification for this pull request, and will review it later. I'm not very keen on introducing a non-core dependency, but I'll see about it.

    Thanks for your efforts.

    Regards,

    -- Shlomi Fish

    1. Toby Inkster author

      It may well be possible to drop Hash::FieldHash and use Tie::RefHash instead. I believe the latter is somewhat slower, but in its favour it's been part of core since 5.004. If you like I can look into changing the implementation to attempt to load Hash::FieldHash, falling back to Tie::RefHash where it's unavailable.