bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Andreas Röhler]

Hi all,

as result of ‘thing-at-point-looking-at’ finally is delivered by 
‘looking-at’,
don't see any sense in calling ‘re-search-forward’ first.

Best,
Andreas

GNU Emacs 27.0.50 (build 1, i686-pc-linux-gnu, GTK+ Version 3.14.5) of 
2019-04-29

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 364 bytes --]

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

> as result of ‘thing-at-point-looking-at’ finally is delivered by
> ‘looking-at’,
> don't see any sense in calling ‘re-search-forward’ first.

The test added below fails if the re-search-forward in
thing-at-point-looking-at is commented out.  Does that tell you what the
"sense" is?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1120 bytes --]

From 79b55e8e6dfee9cba9e464860546dbab2cdd36d8 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 13 May 2019 08:39:00 -0400
Subject: [PATCH] ; Add thing-at-point-looking-at test (Bug#35708)

* test/lisp/thingatpt-tests.el (thing-at-point-looking-at): New test.
---
 test/lisp/thingatpt-tests.el | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/test/lisp/thingatpt-tests.el b/test/lisp/thingatpt-tests.el
index 452fcc6895..339a4a4b81 100644
--- a/test/lisp/thingatpt-tests.el
+++ b/test/lisp/thingatpt-tests.el
@@ -131,4 +131,15 @@ (ert-deftest thing-at-point-url-in-comment ()
     (goto-char 23)
     (should (equal (thing-at-point 'url) "http://foo/bar(baz)"))))
 
+(ert-deftest thing-at-point-looking-at ()
+  (with-temp-buffer
+    (insert "1abcd 2abcd 3abcd")
+    (goto-char (point-min))
+    (let ((m2 (progn (search-forward "2abcd")
+                     (match-data))))
+      (goto-char (point-min))
+      (search-forward "2ab")
+      (should (thing-at-point-looking-at "2abcd"))
+      (should (equal (match-data) m2)))))
+
 ;;; thingatpt.el ends here
-- 
2.11.0

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Andreas Röhler]

On 13.05.19 14:41, Noam Postavsky wrote:
> Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
>
>> as result of ‘thing-at-point-looking-at’ finally is delivered by
>> ‘looking-at’,
>> don't see any sense in calling ‘re-search-forward’ first.
> The test added below fails if the re-search-forward in
> thing-at-point-looking-at is commented out.  Does that tell you what the
> "sense" is?
>

Thought at something like below, which should pass the test:

(defun ar-thing-at-point-looking-at (regexp)
   "Return t if regexp matches at or before point, nil otherwise."
   (save-excursion
       (while (not (or (looking-at regexp)(bolp)))
       (forward-char -1))
       (looking-at regexp)))

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[npostavs]

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

> Thought at something like below, which should pass the test:
>
> (defun ar-thing-at-point-looking-at (regexp)
>   "Return t if regexp matches at or before point, nil otherwise."
>   (save-excursion
>       (while (not (or (looking-at regexp)(bolp)))
>       (forward-char -1))
>       (looking-at regexp)))

I think it's an optimization to use re-search-backward instead of moving
on character at a time and calling looking-at in lisp.

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Andreas Röhler]

On 13.05.19 21:25, npostavs@gmail.com wrote:
> Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
>
>> Thought at something like below, which should pass the test:
>>
>> (defun ar-thing-at-point-looking-at (regexp)
>>    "Return t if regexp matches at or before point, nil otherwise."
>>    (save-excursion
>>        (while (not (or (looking-at regexp)(bolp)))
>>        (forward-char -1))
>>        (looking-at regexp)))
> I think it's an optimization to use re-search-backward instead of moving
> on character at a time and calling looking-at in lisp.
>
>

Hmm, current thing-at-point-looking-at might be slow with large buffers. 
The slightly modified test should reveal it:

(ert-deftest thing-at-point-looking-at-2 ()
   (with-temp-buffer
     (insert "1abcd 222abcd")
     (dotimes (_ 99999) (insert " asdf "))
     (goto-char (point-min))
     (let ((m2 (progn (search-forward "2abcd")
                      (match-data))))
       (goto-char (point-min))
       (search-forward "2ab")
       (should (thing-at-point-looking-at "2abcd"))
       (should (equal (match-data) m2)))))

But let me correct the alternative delivered, as it didn't match before 
point:

(defun ar-regexp-atpt (regexp)

"Return t if REGEXP matches at or before point, nil otherwise.

Changes match-data"
   (save-excursion
     (if (looking-at regexp)
     (while
         (save-excursion
           (and (not (bobp))
            (progn (backward-char) (looking-at regexp)))))
       (while (not (or (bobp) (backward-char) (looking-at regexp))))
       (ar-regexp-atpt regexp))
     (looking-at regexp)))

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Andreas Röhler]

On 14.05.19 12:02, Andreas Röhler wrote:
>
> On 13.05.19 21:25, npostavs@gmail.com wrote:
>> Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
>>
>>> Thought at something like below, which should pass the test:
>>>
>>> (defun ar-thing-at-point-looking-at (regexp)
>>>    "Return t if regexp matches at or before point, nil otherwise."
>>>    (save-excursion
>>>        (while (not (or (looking-at regexp)(bolp)))
>>>        (forward-char -1))
>>>        (looking-at regexp)))
>> I think it's an optimization to use re-search-backward instead of moving
>> on character at a time and calling looking-at in lisp.
>>
>>
>
> Hmm, current thing-at-point-looking-at might be slow with large 
> buffers. The slightly modified test should reveal it:
>
> (ert-deftest thing-at-point-looking-at-2 ()
>   (with-temp-buffer
>     (insert "1abcd 222abcd")
>     (dotimes (_ 99999) (insert " asdf "))
>     (goto-char (point-min))
>     (let ((m2 (progn (search-forward "2abcd")
>                      (match-data))))
>       (goto-char (point-min))
>       (search-forward "2ab")
>       (should (thing-at-point-looking-at "2abcd"))
>       (should (equal (match-data) m2)))))
>
> But let me correct the alternative delivered, as it didn't match 
> before point:
>
> (defun ar-regexp-atpt (regexp)
>
> "Return t if REGEXP matches at or before point, nil otherwise.
>
> Changes match-data"
>   (save-excursion
>     (if (looking-at regexp)
>     (while
>         (save-excursion
>           (and (not (bobp))
>            (progn (backward-char) (looking-at regexp)))))
>       (while (not (or (bobp) (backward-char) (looking-at regexp))))
>       (ar-regexp-atpt regexp))
>     (looking-at regexp)))
>
>
>
>

Another fix, as a bug showed up when testing (ar-regexp-atpt "[a-z]+"):

(defun ar-regexp-atpt (regexp)
   "Return t if REGEXP matches at or before point, nil otherwise.

Changes match-data"
   (save-excursion
     (if (looking-at regexp)
     (while
         (and (not (bobp))
          (or (progn (backward-char) (looking-at regexp))
              (forward-char 1))))
       (while (not (or (bobp) (backward-char) (looking-at regexp))))
       (ar-regexp-atpt regexp))
     (looking-at regexp)))

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[npostavs]

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

>> Hmm, current thing-at-point-looking-at might be slow with large
>> buffers. The slightly modified test should reveal it:
>>
>> (ert-deftest thing-at-point-looking-at-2 ()
>>   (with-temp-buffer
>>     (insert "1abcd 222abcd")
>>     (dotimes (_ 99999) (insert " asdf "))
>>     (goto-char (point-min))

>>       (search-forward "2ab")
>>       (should (thing-at-point-looking-at "2abcd"))

Yes, in this case, since the loop over looking-at only needs to iterate
twice, so it will be faster.  But what about when there is no match?
E.g.,

(with-temp-buffer
  (dotimes (_ 99999) (insert " asdf "))
  (goto-char (point-max))
  (list :ar-regexp-atpt (benchmark-run (ar-regexp-atpt "foo"))
        :thing-at-point-looking-at (benchmark-run (thing-at-point-looking-at "foo"))))

> Another fix, as a bug showed up when testing (ar-regexp-atpt "[a-z]+"):
>
> (defun ar-regexp-atpt (regexp)
>   "Return t if REGEXP matches at or before point, nil otherwise.
>
> Changes match-data"
>   (save-excursion
>     (if (looking-at regexp)
>     (while
>         (and (not (bobp))
>          (or (progn (backward-char) (looking-at regexp))
>              (forward-char 1))))
>       (while (not (or (bobp) (backward-char) (looking-at regexp))))
>       (ar-regexp-atpt regexp))

What's this recursive call for?  It triggers (error "Lisp nesting
exceeds ‘max-lisp-eval-depth’") in the benchmark above.

>     (looking-at regexp)))

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Andreas Röhler]

Am 14.05.19 um 16:34 schrieb npostavs@gmail.com:
> Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
>
>>> Hmm, current thing-at-point-looking-at might be slow with large
>>> buffers. The slightly modified test should reveal it:
>>>
>>> (ert-deftest thing-at-point-looking-at-2 ()
>>>    (with-temp-buffer
>>>      (insert "1abcd 222abcd")
>>>      (dotimes (_ 99999) (insert " asdf "))
>>>      (goto-char (point-min))
>>>        (search-forward "2ab")
>>>        (should (thing-at-point-looking-at "2abcd"))
> Yes, in this case, since the loop over looking-at only needs to iterate
> twice, so it will be faster.  But what about when there is no match?
> E.g.,
>
> (with-temp-buffer
>    (dotimes (_ 99999) (insert " asdf "))
>    (goto-char (point-max))
>    (list :ar-regexp-atpt (benchmark-run (ar-regexp-atpt "foo"))
>          :thing-at-point-looking-at (benchmark-run (thing-at-point-looking-at "foo"))))
>
>> Another fix, as a bug showed up when testing (ar-regexp-atpt "[a-z]+"):
>>
>> (defun ar-regexp-atpt (regexp)
>>    "Return t if REGEXP matches at or before point, nil otherwise.
>>
>> Changes match-data"
>>    (save-excursion
>>      (if (looking-at regexp)
>>      (while
>>          (and (not (bobp))
>>           (or (progn (backward-char) (looking-at regexp))
>>               (forward-char 1))))
>>        (while (not (or (bobp) (backward-char) (looking-at regexp))))
>>        (ar-regexp-atpt regexp))
> What's this recursive call for?  It triggers (error "Lisp nesting
> exceeds ‘max-lisp-eval-depth’") in the benchmark above.
>
>>      (looking-at regexp)))


The recursive call needed a guard:     (unless (bobp)

It is called after function went backward while not looking-at matches,

Now the result for the 99999 is

(:ar-regexp-atpt (0.774574453 0 0.0) :thing-at-point-looking-at 
(0.000798669 0 0.0))


The fixed form:

(defun ar-regexp-atpt (regexp)
   "Return t if REGEXP matches at or before point, nil otherwise.

Changes match-data"
   (save-excursion
     (if (looking-at regexp)
     (while
         (and (not (bobp))
          (or (progn (backward-char) (looking-at regexp))
              (forward-char 1))))
       (while (not (or (bobp) (backward-char) (looking-at regexp))))
       (unless (bobp) (ar-regexp-atpt regexp)))
     (looking-at regexp)))

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Andreas Röhler]

[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]


Am 15.05.19 um 08:59 schrieb Andreas Röhler:
>
> Am 14.05.19 um 16:34 schrieb npostavs@gmail.com:
>> Andreas Röhler <andreas.roehler@easy-emacs.de> writes:
>>
>>>> Hmm, current thing-at-point-looking-at might be slow with large
>>>> buffers. The slightly modified test should reveal it:
>>>>
>>>> (ert-deftest thing-at-point-looking-at-2 ()
>>>>    (with-temp-buffer
>>>>      (insert "1abcd 222abcd")
>>>>      (dotimes (_ 99999) (insert " asdf "))
>>>>      (goto-char (point-min))
>>>>        (search-forward "2ab")
>>>>        (should (thing-at-point-looking-at "2abcd"))
>> Yes, in this case, since the loop over looking-at only needs to iterate
>> twice, so it will be faster.  But what about when there is no match?
>> E.g.,
>>
>> (with-temp-buffer
>>    (dotimes (_ 99999) (insert " asdf "))
>>    (goto-char (point-max))
>>    (list :ar-regexp-atpt (benchmark-run (ar-regexp-atpt "foo"))
>>          :thing-at-point-looking-at (benchmark-run 
>> (thing-at-point-looking-at "foo"))))
>>
>>> Another fix, as a bug showed up when testing (ar-regexp-atpt "[a-z]+"):
>>>
>>> (defun ar-regexp-atpt (regexp)
>>>    "Return t if REGEXP matches at or before point, nil otherwise.
>>>
>>> Changes match-data"
>>>    (save-excursion
>>>      (if (looking-at regexp)
>>>      (while
>>>          (and (not (bobp))
>>>           (or (progn (backward-char) (looking-at regexp))
>>>               (forward-char 1))))
>>>        (while (not (or (bobp) (backward-char) (looking-at regexp))))
>>>        (ar-regexp-atpt regexp))
>> What's this recursive call for?  It triggers (error "Lisp nesting
>> exceeds ‘max-lisp-eval-depth’") in the benchmark above.
>>
>>>      (looking-at regexp)))
>
>
> The recursive call needed a guard:     (unless (bobp)
>
> It is called after function went backward while not looking-at matches,
>
> Now the result for the 99999 is
>
> (:ar-regexp-atpt (0.774574453 0 0.0) :thing-at-point-looking-at 
> (0.000798669 0 0.0))
>
>
> The fixed form:
>
>
Make sure match pos includes cursor pos:


(defun ar-regexp-atpt (regexp)

   "Return t if REGEXP matches at or before point, nil otherwise.
Changes match-data"
   (save-excursion
     (let ((orig (point)))
       (if (looking-at regexp)
       (while
           (and (not (bobp))
            (or (progn (backward-char) (looking-at regexp))
                (forward-char 1))))
     (while (not (or (bobp) (backward-char) (looking-at regexp))))
     (unless (bobp) (ar-regexp-atpt regexp)))
       (and
        (looking-at regexp)
        (<= (match-beginning 0) orig)
        (>= (match-end 0) orig)))))



[-- Attachment #2: Type: text/html, Size: 4943 bytes --]

bug#35708: [27.0.50]: thingatpt.el, thing-at-point-looking-at redundant

[Noam Postavsky]

tags 35708 notabug wontfix
close 35708
quit

Andreas Röhler <andreas.roehler@easy-emacs.de> writes:

> Now the result for the 99999 is
>
> (:ar-regexp-atpt (0.774574453 0 0.0)
>  :thing-at-point-looking-at (0.000798669 0 0.0))

I trust that explains well enough why the current implementation is
used.