Re: [PATCH] xml-escape-region

[Stefan Monnier]

>>> +;;;##autoload
>>> +(defun xml-escape-region (beg end)
>>> +  (interactive "*r")
>>> +  (let ((escaped (xml-escape-string (buffer-substring beg end))))
>>> +    (delete-region beg end)
>>> +    (insert escaped)))
>> 
>> I'd rather not autoload such a function.

> Do you mean that it should be loaded all the time, or that the user should
> have to explicitly load xml.el before using the function?

Yes.

> If the latter, then that would make binding it to a key
> less convenient.

Hmm... didn't notice you defined it as a command.  How often/when do you
need to use/bind such a command other than in an sgml/xml-related file
(where the major mode might decide to preload such a command)?

>> But more importantly, this implementation is very inefficient.
>> xml-escape-string itself is rather inefficient except for short
>> strings; this is OK for its current uses, but for xml-escape-region
>> it's definitely not good (i.e. only usable for small regions).

> How's this? It's O(N) in the amount of text escaped.

Much better, thank you.

>   (let ((search-re (mapconcat #'regexp-quote
>                               (mapcar #'cdr xml-entity-alist)
>                               "\\|"))

Rather than a big \| of single chars, why not make a [...] regexp?
If you use regexp-opt, it should happen automatically.

Actually, now that I look at it, xml-entity-alist is poorly defined.
Instead of being a list of pairs of string and string (where the second
string is always of size 1), it should be a list of pairs of string
and char.  Also this code is also applicable to sgml and there's related
code in sgml-mode.el.  If someone wants to consolidate, that would
be welcome.

>     (save-excursion
>       (goto-char beg)
>       (while (re-search-forward search-re end t)
>         (replace-match (concat "&"
>                                (car (rassoc (match-string 0)
>                                             xml-entity-alist))
>                                ";"))))))

If you use a backward-search, you don't need to turn `end' (nor `start')
into a marker.


        Stefan