> On 27 Apr 2018, at 05:06, Branko Čibej <br...@apache.org> wrote:
>
> On 27.04.2018 11:30, Nick Kew wrote:
>>> In my use cases, the search may often be strings, but not always, so I
>>> thought that maybe APR could/should provide something more generic. The
>>> above functions could then be rewritten to use this new function. Here are
>>> my thoughts:
>>>
>>> 1. The search item (needle) can be anything rather than just a string as
>>> above.
>>> 2. The caller provides a callback that compares the values to needle in the
>>> same way strcmp()/strcasecmp() do. That is, 0 means "equal". In fact,
>>> strcmp()/strcasecmp() can -- and probably often will -- be used as the
>>> callback for strings.
>> There’s a bit of a mismatch between that and the “can be anything”.
>> The latter would call for a size parameter that could be passed to memcmp,
>> as well as of course strncmp/strncasecmp.
>
> You can't just use any of the string comparison functions directly
> anyway: first because the signature is wrong, and second because there's
> no requirement that array elements are pointers.
Nick - I don't expect that comp would ever do just a memcmp...unless that's how
someone implements it, but then that implementation would have to know what
kind of data (and the size) it is dealing with. The comparator would be a
custom written function that expects int or char* or struct* or _____ and do
whatever it considers an equality comparison. It may receive struct*s and only
compare one or two members to consider them "equal" in the comparator's context.
Branko - First, I can use them directly with either a compiler warning
(implicit cast char* to void*) or an explicit cast. Second, this does not
require that the elements be pointers. If they are char*, though, strcmp and
strcasecmp work fine.
>>> 3. A start (input) and index (output) parameter can be used to iterate and
>>> find all occurrences.
>>> What do others think? Is there value in this, or is it just me?
>> I’ve had occasion to search an array, though I don’t think I’ve needed
>> anything more
>> generic than the HTTPD functions you name above.
>>
>>> +typedef int (apr_array_find_comparison_callback_fn_t)(const void *x, const
>>> void *y);
>> Minor quibble: the name seems quite tortuously overdescriptive!
Agreed, but it's not something that really gets used much, though. I just based
it off the apr_table_do callback's name.
> Major quibble: The comparator should take a parameter that receives the
> array element size. Careful implementations will either use that size or
> check that it's the correct size. APR arrays can contain whole
> structures, not just pointers.
Again, the comparator has to be smart. It should know what it is receiving.
They may be ints, structs, strings, whatever. It can and should cast as
necessary. The void* is just a way of saying "I'm passing you something. You
have to know how to handle it." It may or may not be a pointer. I have
successfully tested it with char* (using strcmp/strcasecmp) as well as int and
char using custom comp callback functions. They all worked as expected.
>
>>> + for (i = start; (i < arr->nelts) && comp(needle, *(void **)(arr->elts +
>>> (arr->elt_size * i))); i++) {
>>> + /* empty */
>>> + }
>
> The comparator should receive the pointer to the array element, not the
> array element interpreted as a pointer! See above, array elements don't
> have to be pointers. Why else do you think apr_array_t::elt_size exists?
> Or the APR_ARRAY_IDX() and APR_ARRAY_PUSH() macros?
Again, see above. It is only void* to say "I am passing you something, but
*you* know what it really is."
>> Why not make that much more readable and put the substance inside the loop?
>> for (i …) {
>> if (!comp(…)) {
>> set *index;
>> set return value;
>> break;
>> }
>> }
>
> +1
Agreed:
diff -r 335976d9e7cb tables/apr_tables.c
--- a/tables/apr_tables.c Wed Apr 04 16:41:28 2018 +0000
+++ b/tables/apr_tables.c Fri Apr 27 11:40:48 2018 -0500
@@ -281,6 +281,31 @@
return res;
}
+APR_DECLARE(void *) apr_array_find(apr_array_header_t *arr,
+ const void *needle,
+ apr_array_find_comparison_callback_fn_t
*comp,
+ int start,
+ int *index)
+{
+ int i;
+
+ if (!arr || !needle || !comp || (start < 0)) {
+ return NULL;
+ }
+
+ for (i = start; i < arr->nelts; i++) {
+ if (comp(needle, *(void **)(arr->elts + (arr->elt_size * i))) == 0) {
+ if (index) {
+ *index = i;
+ }
+
+ return *(void **)(arr->elts + (arr->elt_size * i));
+ }
+ }
+
+ return NULL;
+}
+
/*****************************************************************
*
