php bindings - get_values for ValueCountMatchSpy return resource

[overgroove]

Attached is a patch that modifies util.i and adds a typedef for the std::map that is returned from that function.

[overgroove]

Patch file for util.i

[richard]

Thanks for the patch. A few notes and questions:

• Can the call to add_assoc_long() return an error? Googling shows me that it has "int" as a return type, and I imagine it can fail for various reasons (it must allocate some memory, for a start), but I can't find any documentation of what the return type means.

• Does add_assoc_long() take a copy of the string (or, indeed, does it allow it to be modified). Again, I couldn't find any documentation of this, but if it doesn't take a copy, we'll be liable to get segfaults and buffer overruns if the array is accessed after the MatchSpy? has been deallocated.

• Does this patch actually make get_values() work usably for you? Ideally, we'd add a test to smoketest.php to ensure that it works, and continues to work.

We're (well, I'm) just in the process of drafting a policy for the license which contributors need to give for their code to be included in the project; this patch is big enough that we'd need your agreement to license the code appropriately, assuming it works and we therefore want to merge it to trunk. Basically, we need your permission to distribute the code under both the GPL and the MIT/X license. Feel free to ask on IRC if you need details about this.

Marking this ticket for consideration for 1.1.4.

[olly]

Cleaned up patch

[olly]

Cleaned up patch attached - fixes formatting, uses add_assoc_long_ex() to correctly handle terms with embedded zero bytes, and removes the pointless #define and random added blank line.

Looking at the PHP source, add_assoc_long() returns SUCCESS or FAILURE, but since that appears not to be documented at all, nobody else in the world actually checks its return value. I've ignored that for now. It isn't obvious if the arguments are copied or not from the code - testing would probably be an easier way to check that. Pretty much all calls in the wild seem to pass a constant string.

This does really test coverage though, so not applying for now.

FWIW, the FSF wouldn't regard a patch of this size as significant for copyright purposes:

 http://www.gnu.org/prep/maintain/maintain.html#Legally-Significant

[overgroove]

Cool, thanks for the cleanup. Maybe I'll messing around and try creating some situation where add_assoc_long would fail and seeing how it actually behaves. Also, I read somewhere from--an unreliable source I'm sure--that add_assoc_long is just an alias to add_assoc_long_ex. Although, if you were looking at the source and didn't see that, then said source must be wrong, outdated, or both.

In regards to having this work for me, it did work for me. One thing I'd mention is that is that I get an empty array back when the std::map is empty. This is expected but I'm not sure what your preference is for what to return in that situation. An empty array works fine for me.

[overgroove]

Hmm, possible confusion: by "said source" above, I mean the unreliable source from the previous line.

[olly]

add_assoc_long() is indeed just a macro which calls add_assoc_long_ex(), but the length it uses is strlen(p) which will truncate the term if it contains zero bytes. By calling add_assoc_long_ex() explicitly, we can pass term.length() for the length and preserve any zero bytes in term.

Even if we don't care about zero bytes in term, it's likely to be slightly more efficient to use the already known length rather than recomputing it.

[richard]

I'll try and put together a test case for this shortly.

[richard]

Fixed in r13897. The patch applied was essentially util.i.mk2.patch as attached, except that the length supplied to add_assoc_long_ex() needs to be 1 + keylength, not keylength: another unusual PHP API there.

[richard]

overgroove: if you would like to be added to the list of contributors to xapian (in xapian-core/AUTHORS), please let us know your name. Thanks.