Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Lars Magne Ingebrigtsen]

Juanma Barranquero <lekktu@gmail.com> writes:

> +2013-08-12  Juanma Barranquero  <lekktu@gmail.com>
> +
> +	* xml.el (xml-parse-tag-1): Use looking-at (this reverts change in
> +	revno:113793, which breaks the test suite).
> +	https://lists.gnu.org/archive/html/emacs-devel/2013-08/msg00263.html

[...]

> -	    (while (not (looking-at-p end))
> +	    (while (not (looking-at end))

Wouldn't it be better to fix the bogus test instead of changing
apparently perfectly good code (for the (slightly) worse)?

That's the danger of test suites.  >"?

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Juanma Barranquero]

On Mon, Aug 12, 2013 at 5:32 PM, Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:

>> -         (while (not (looking-at-p end))
>> +         (while (not (looking-at end))
>
> Wouldn't it be better to fix the bogus test instead of changing
> apparently perfectly good code (for the (slightly) worse)?

I'd tend to agree. In this case, an apparent predicate test (even
using (not ...) to reinforce that idea) has a follow-up match-data use
almost 30 lines afterwards; to make things worse,
the code in between (inside (while ...)) also uses string matching
(which does not affect the test, but produces some cognitive load when
reading it). Ugly, I'd say.

> That's the danger of test suites.  >"?

All in all, a small one, against the advantage of having working code ;-)

    J

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Lars Magne Ingebrigtsen]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Mon, Aug 12, 2013 at 5:32 PM, Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:
>
>>> -         (while (not (looking-at-p end))
>>> +         (while (not (looking-at end))
>>
>> Wouldn't it be better to fix the bogus test instead of changing
>> apparently perfectly good code (for the (slightly) worse)?
>
> I'd tend to agree. In this case, an apparent predicate test (even
> using (not ...) to reinforce that idea) has a follow-up match-data use
> almost 30 lines afterwards; to make things worse,
> the code in between (inside (while ...)) also uses string matching
> (which does not affect the test, but produces some cognitive load when
> reading it). Ugly, I'd say.

Oh, the code really didn't work with `looking-at-p'?  Then I guess I
misinterpreted the checkin message.  :-)

	    (while (not (looking-at-p end))

[ snipped 27 lines, some of which do alter the match data ]

	    ;; Move point past the end-tag.
	    (goto-char (match-end 0))

Yeah, that doesn't look...  good...

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Juanma Barranquero]

On Mon, Aug 12, 2013 at 6:18 PM, Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:

> Oh, the code really didn't work with `looking-at-p'?  Then I guess I
> misinterpreted the checkin message.  :-)

Well, I assumed that saying that it broke the test implied that it
broke the code. Will try to be clearer next time.

> Yeah, that doesn't look...  good...

No, it doesn't.

    J

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Glenn Morris]

Lars Magne Ingebrigtsen wrote:

> Oh, the code really didn't work with `looking-at-p'?  Then I guess I
> misinterpreted the checkin message.  :-)

What's that you say... "gee, my eyes have been opened as to the value of
test-suites, and I will start writing one for Gnus forthwith"? ;)

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Lars Magne Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> What's that you say... "gee, my eyes have been opened as to the value of
> test-suites, and I will start writing one for Gnus forthwith"? ;)

That's what I've been doing the past half hour!

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Stefan Monnier]

>> What's that you say... "gee, my eyes have been opened as to the value of
>> test-suites, and I will start writing one for Gnus forthwith"? ;)
> That's what I've been doing the past half hour!

Not sure if that means you've been saying it reeeaalllly sslllooowww or
repeating it over and over, but in case it doesn't sound like a useful
use of your time.

BTW, another thing to learn for this is that we shouldn't bother to
replace looking-at with looking-at-p.

It can be handy to use looking-at-p if it saves us from using
save-match-data, but other than that it's not worth the trouble.


        Stefan

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Juanma Barranquero]

On Tue, Aug 13, 2013 at 3:46 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> BTW, another thing to learn for this is that we shouldn't bother to
> replace looking-at with looking-at-p.
>
> It can be handy to use looking-at-p if it saves us from using
> save-match-data, but other than that it's not worth the trouble.

I disagree, as I explained before, because I think the greatest
benefit of string-match-p / looking-at-p is that they are predicates.
They're easier to interpret, because you can then forget about ugly
cases like the one which prompted this thread, were the call to the
match-data setting function and its use are quite far apart; that's
error prone, not only to the reader, but to the maintainer that can
easily insert code in between without realizing the mistake.

I don't go out of my way to do the switch, but if I'm fixing other
things or typos or whatever and I find them, and they clearly are used
as predicates, I change them. Every now and them I overlook a use of
match-data, but usually the error is promptly caught.

   J

Re: trunk r113818: lisp/xml.el (xml-parse-tag-1): Use looking-at.

[Lars Magne Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Not sure if that means you've been saying it reeeaalllly sslllooowww or
> repeating it over and over, but in case it doesn't sound like a useful
> use of your time.

Juuuust kidding.

-- 
(domestic pets only, the antidote for overdose, milk.)
  No Gnus T-Shirt for sale: http://ingebrigtsen.no/no.php
  and http://lars.ingebrigtsen.no/2013/08/twenty-years-of-september.html