Re: trunk r117396: Do not allow out-of-range character position in Fcompare_strings.

Re: trunk r117396: Do not allow out-of-range character position in Fcompare_strings.

[Dmitry]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> ------------------------------------------------------------
> revno: 117396
> revision-id: dmantipov@yandex.ru-20140625103651-k76n6ekn8asbgf5p
> parent: rgm@gnu.org-20140625101741-09kj8husqtljm4u0
> committer: Dmitry Antipov <dmantipov@yandex.ru>
> branch nick: trunk
> timestamp: Wed 2014-06-25 14:36:51 +0400
> message:
>   Do not allow out-of-range character position in Fcompare_strings.
>   * src/fns.c (validate_subarray): Add prototype.
>   (Fcompare_substring): Use validate_subarray to check ranges.
>   Adjust comment to mention that the semantics was changed.  Also see
>   http://lists.gnu.org/archive/html/emacs-devel/2014-06/msg00447.html.

IIUC, this small discussion ended with "it's too late to fix".  Why did
you make the change, then?

And if it's okay to change the semantics, why not change it the other
way, relaxing `substring' requirements?

There was a thread not too long ago requesting that.

Re: trunk r117396: Do not allow out-of-range character position in Fcompare_strings.

[Dmitry Antipov]

On 06/25/2014 05:22 PM, Dmitry wrote:
> IIUC, this small discussion ended with "it's too late to fix".  Why did
> you make the change, then?

"Too late" for emacs-24, which is in pretest stage.

> And if it's okay to change the semantics, why not change it the other
> way, relaxing `substring' requirements?

Why relaxing? IMO relaxing just means more error-prone code (imagine
an `aset' which silently ignores writing an element beyond array end,
or `aref' which always returns nil in that case).

Anyway, you can defsubst a tiny wrapper to relax if you need.

Dmitry

Re: trunk r117396: Do not allow out-of-range character position in Fcompare_strings.

[Stefan Monnier]

>> IIUC, this small discussion ended with "it's too late to fix".  Why did
>> you make the change, then?
> "Too late" for emacs-24, which is in pretest stage.

That was a misunderstanding: the discussion has never been about
emacs-24 AFAICT since it's not about fixing a bug.

>> And if it's okay to change the semantics, why not change it the other
>> way, relaxing `substring' requirements?
> Why relaxing?

It can be convenient.  There's always a tension between checking args
sanity and providing a useful fallback behavior for
"non-canonical" values.

> IMO relaxing just means more error-prone code (imagine
> an `aset' which silently ignores writing an element beyond array end,
> or `aref' which always returns nil in that case).

We don't have the behavior for aref, but we do have it for nth and
gethash and people frequently rely on this behavior.

> Anyway, you can defsubst a tiny wrapper to relax if you need.

Most of your patch is good.  The only problematic part is the one that
signals an error for greater-than-length values.  Adding a wrapper won't
solve that problem, since the main issue is not that the new semantic is
bad but that it breaks backward compatibility.

Now that it's installed, I guess we can try to run with it for a while,
but I expect we'll get many bug reports from it because external
packages rely on that behavior (just like bundled packages relied on it,
or even more so since many external packages don't use string-prefix-p
because it didn't exist back then).


        Stefan

Re: trunk r117396: Do not allow out-of-range character position in Fcompare_strings.

[Dmitry Antipov]

On 06/25/2014 07:17 PM, Stefan Monnier wrote:

> That was a misunderstanding: the discussion has never been about
> emacs-24 AFAICT since it's not about fixing a bug.

May be I just misunderstood the development/release process as a whole,
but what "it's too late" means for the trunk?

> We don't have the behavior for aref, but we do have it for nth and
> gethash and people frequently rely on this behavior.

Hm...lists are mutable - you can concatenate A and B (with setcdr)
and get changed A but not the copy of A + B. On the other side, I always
consider strings and vectors as "less mutable", which implies more strict
checking (as with aref and aset).

> Now that it's installed, I guess we can try to run with it for a while,
> but I expect we'll get many bug reports from it because external
> packages rely on that behavior (just like bundled packages relied on it,
> or even more so since many external packages don't use string-prefix-p
> because it didn't exist back then).

In ELPA, I don't see too much users of compare-strings:

$ grep -nHR "(compare-strings " .
packages/company/company.el:1093:              (not (eq t (compare-strings (car candidates) nil nil
packages/vlf/vlf-ediff.el:323:           (eq t (compare-strings suffix nil nil string start-pos nil
packages/gnugo/gnugo.el:290:  (compare-strings s1 beg1 nil s2 beg2 nil))

I can check other external packages as well. Emacspeak?

Dmitry

Re: trunk r117396: Do not allow out-of-range character position in Fcompare_strings.

[Dmitry Gutov]

On 06/25/2014 06:57 PM, Dmitry Antipov wrote:

> May be I just misunderstood the development/release process as a whole,
> but what "it's too late" means for the trunk?

Too late to changes APIs that have been in use for a long time, I suppose.

>> We don't have the behavior for aref, but we do have it for nth and
>> gethash and people frequently rely on this behavior.
>
> Hm...lists are mutable - you can concatenate A and B (with setcdr)
> and get changed A but not the copy of A + B. On the other side, I always
> consider strings and vectors as "less mutable", which implies more strict
> checking (as with aref and aset).

To avoid rehashing the same argument, here's the previous discussion: 
http://lists.gnu.org/archive/html/emacs-devel/2012-07/msg00313.html

> I can check other external packages as well. Emacspeak?

You can look here, too: 
https://github.com/search?l=emacs-lisp&q=compare-strings&ref=cmdform&type=Code