On 2010-10-20 at 00:52 +0100, Mike Cardwell wrote:
> http://github.com/mikecardwell/Exim/commit/b4ef92d69ac58a46cbb0babb93d688267fbed443
>
> Provides a ${index{string}{substring}} function to return the location
> of a string within a string. Examples:
Generally like it. My main commentary is at the bikeshed-painting
level, so I'll throw it out there, but am not going to protest if you
ignore me.
Note that Exim doesn't really do much sequential work with variables
holding results, although it can of course be so abused, so I'm curious
about the desire to use an expansion operator instead of an expansion
condition for the basic check. What's the use-case?
At a code-review level: multiple case statements inside a switch which
fall through should usually have a comment stating that this is explicit
and intentional. Looking for prior art in expand.c, I see both /* Fall
through */ and, as counter-example, ECOND_ISIP/ECOND_ISIP4/ECOND_ISIP6.
I'd argue that it's clear that the ECOND_ISIP* cases are being handled
in common, whereas in this case, I had to go check the documentation for
read_subs() to see the return values, to confirm that the 2 and 3 values
could reasonably be treated the same and this was not accidental code
omission.
Is %d cross-platform safe formatting for a ptrdiff_t type? I'd be
"impressed" if an Exim string ever holds enough data to have to worry
about wrap-around or overflow of 32-bit here, but it might be safest to
use %ld and a (long int) cast. FreeBSD uses 64-bit signed ptrdiff_t and
that's a 'long'. My vague recollection is that this is normal for
64-bit Unix. 'long long' looks like being more portably correct. And
there's no PRIi* macro for ptrdiff_t. *sigh*
> condition = ${if >{${index{$h_Subject:}{$sender_address}}}{-1}}
>
> You could use ${match} but then you'd have to worry about special regex
> characters in the key such as '.'.
Well, that's what \Q${untrusted}\E is for.
> Comments?
Index is a generic term, also applicable to databases, etc. There's no
namespacing in Exim, so I'm ... "looking askew" at the name. Yeah,
bikeshed painting, sorry. Exim's not perfect here, and since Exim is
string-based, string tends to be the default data-type assumption.
A number of the Exim expansion operators for pure-string issues contain
'str' as part of the name. But not enough to declare this a hard rule.
Also, Python has both .find() and .index() string methods, where .find()
returns -1 on failure and .index() raises an exception on failure.
Whereas Perl return base_index-1, which is typically -1. Ruby returns
nil. This is a mess. Perhaps I've just been doing too much Python
recently and that's clouding my opinion of the naming.
strindex(), strstr() [for the C programmers, but which then leads to
strcasestr() vs the Exim-model indexi, which you rightly chose],
something.
Or freely discard what I've said about the naming. :) I just want the
issue to have been considered, even if you decide to stick with the name
you chose.
-Phil
