Re: [Emacs-diffs] master 75d9a55: Fix bug 32543

Re: [Emacs-diffs] master 75d9a55: Fix bug 32543

[Stefan Monnier]

> +(defun occur--parse-occur-buffer()
> +  "Retrieve a list of the form (BEG END ORIG-LINE BUFFER).
> +BEG and END define the region.
> +ORIG-LINE and BUFFER are the line and the buffer from which
> +the user called `occur'."
> +  (save-excursion
> +    (goto-char (point-min))
> +    (let ((buffer (get-text-property (point-at-bol) 'occur-title))
> +          (beg-pos (get-text-property (point-at-bol) 'region-start))
> +          (end-pos (get-text-property (point-at-bol) 'region-end))
> +          (orig-line (get-text-property (point-at-bol) 'current-line))
> +          beg-line end-line)
> +      (list beg-pos end-pos orig-line buffer))))

I'm curious here:
- Why use (point-at-bol) since we just moved to (point-min) and hence we
  know that (point-at-bol) == (point-min) == (point)?
- why store this info in text-properties rather than in buffer-local variables?

> +    (let* ((region (occur--parse-occur-buffer))
> +           (region-start (nth 0 region))
> +           (region-end (nth 1 region))
> +           (orig-line (nth 2 region))
> +           (buffer (nth 3 region))
> +           (regexp (car occur-revert-arguments)))

Here you could have used pcase-let:

    (pcase-let ((`(,region-start ,region-end ,orig-line ,buffer)
                 (occur--parse-occur-buffer))
                (regexp (car occur-revert-arguments)))

> +      (with-current-buffer buffer
> +        (when (wholenump orig-line)
> +          (goto-char 1)

I'd recommend `point-min` instead of 1 here.

> +          (forward-line (1- orig-line)))
> +        (save-excursion
> +          (if region
> +              (occur regexp nil (list (cons region-start region-end)))
> +            (apply 'occur-1 (append occur-revert-arguments (list (buffer-name))))))))))

`region` is the list which holds (among other things) `buffer`, so if we
successfully entered (with-current-buffer buffer ...) I can't see how
`region` could be nil.


        Stefan

Re: [Emacs-diffs] master 75d9a55: Fix bug 32543

[Tino Calancha]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> +(defun occur--parse-occur-buffer()
>> +  "Retrieve a list of the form (BEG END ORIG-LINE BUFFER).
>> +BEG and END define the region.
>> +ORIG-LINE and BUFFER are the line and the buffer from which
>> +the user called `occur'."
>> +  (save-excursion
>> +    (goto-char (point-min))
>> +    (let ((buffer (get-text-property (point-at-bol) 'occur-title))
>> +          (beg-pos (get-text-property (point-at-bol) 'region-start))
>> +          (end-pos (get-text-property (point-at-bol) 'region-end))
>> +          (orig-line (get-text-property (point-at-bol) 'current-line))
>> +          beg-line end-line)
>> +      (list beg-pos end-pos orig-line buffer))))
>
> I'm curious here:
> - Why use (point-at-bol) since we just moved to (point-min) and hence we
>   know that (point-at-bol) == (point-min) == (point)?
In a previous patch I saved the info in one particular position other
that (point-min); finally I saved the info in the entire line as it is
done with the buffer.
> - why store this info in text-properties rather than in buffer-local variables?
I realized the buffer was already saved there with property
`occur-title'; I didn't wanted to scatter around related info.
>> +    (let* ((region (occur--parse-occur-buffer))
>> +           (region-start (nth 0 region))
>> +           (region-end (nth 1 region))
>> +           (orig-line (nth 2 region))
>> +           (buffer (nth 3 region))
>> +           (regexp (car occur-revert-arguments)))
>
> Here you could have used pcase-let:
>
>     (pcase-let ((`(,region-start ,region-end ,orig-line ,buffer)
>                  (occur--parse-occur-buffer))
>                 (regexp (car occur-revert-arguments)))
Nicer. Thank you!

>> +      (with-current-buffer buffer
>> +        (when (wholenump orig-line)
>> +          (goto-char 1)
>
> I'd recommend `point-min` instead of 1 here.
OK.  I always remember the discussion at
https://lists.gnu.org/archive/html/emacs-devel/2009-08/msg00520.html
but I forgot which was the encouraged practice there: in these cases
I follow the 50% rule to reject 0% success ratio (I know, it brings
50% failure ratio: life is full of injustice).

>> +          (forward-line (1- orig-line)))
>> +        (save-excursion
>> +          (if region
>> +              (occur regexp nil (list (cons region-start region-end)))
>> +            (apply 'occur-1 (append occur-revert-arguments (list (buffer-name))))))))))
>
> `region` is the list which holds (among other things) `buffer`, so if we
> successfully entered (with-current-buffer buffer ...) I can't see how
> `region` could be nil.
Good catch!
It looks like my typo.  Probably I wanted here to check if we have
called `occur' before with a non-nil region, i.e., this check:
+          (if (or region-start region-end)

I can push following amend commit:
--8<-----------------------------cut here---------------start------------->8---
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1213,29 +1213,25 @@ occur--parse-occur-buffer
 the user called `occur'."
   (save-excursion
     (goto-char (point-min))
-    (let ((buffer (get-text-property (point-at-bol) 'occur-title))
-          (beg-pos (get-text-property (point-at-bol) 'region-start))
-          (end-pos (get-text-property (point-at-bol) 'region-end))
-          (orig-line (get-text-property (point-at-bol) 'current-line))
-          beg-line end-line)
+    (let ((buffer (get-text-property (point) 'occur-title))
+          (beg-pos (get-text-property (point) 'region-start))
+          (end-pos (get-text-property (point) 'region-end))
+          (orig-line (get-text-property (point) 'current-line)))
       (list beg-pos end-pos orig-line buffer))))
 
 (defun occur-revert-function (_ignore1 _ignore2)
   "Handle `revert-buffer' for Occur mode buffers."
   (if (cdr (nth 2 occur-revert-arguments)) ; multi-occur
       (apply 'occur-1 (append occur-revert-arguments (list (buffer-name))))
-    (let* ((region (occur--parse-occur-buffer))
-           (region-start (nth 0 region))
-           (region-end (nth 1 region))
-           (orig-line (nth 2 region))
-           (buffer (nth 3 region))
-           (regexp (car occur-revert-arguments)))
+    (pcase-let ((`(,region-start ,region-end ,orig-line ,buffer)
+                 (occur--parse-occur-buffer))
+                (regexp (car occur-revert-arguments)))
       (with-current-buffer buffer
         (when (wholenump orig-line)
-          (goto-char 1)
+          (goto-char (point-min))
           (forward-line (1- orig-line)))
         (save-excursion
-          (if region
+          (if (or region-start region-end)
               (occur regexp nil (list (cons region-start region-end)))
             (apply 'occur-1 (append occur-revert-arguments (list (buffer-name))))))))))
 

--8<-----------------------------cut here---------------end--------------->8---

Re: [Emacs-diffs] master 75d9a55: Fix bug 32543

[Stefan Monnier]

>> - why store this info in text-properties rather than in
>>   buffer-local variables?
> I realized the buffer was already saved there with property
> `occur-title'; I didn't wanted to scatter around related info.

Hmmm... do you happen to have some intuition about why the buffer was
saved in a text-property rather than in a buffer-local var?
It's a really odd choice (less efficient, more work for the coder, with
various side-problems like "on which char should I put it", etc...).

[ Also, why is it saved under a name like `occur-title` rather
  than, say `occur-buffer`?  ]

>>> +      (with-current-buffer buffer
>>> +        (when (wholenump orig-line)
>>> +          (goto-char 1)
>>
>> I'd recommend `point-min` instead of 1 here.
> OK.  I always remember the discussion at
> https://lists.gnu.org/archive/html/emacs-devel/2009-08/msg00520.html
> but I forgot which was the encouraged practice there: in these cases
> I follow the 50% rule to reject 0% success ratio (I know, it brings
> 50% failure ratio: life is full of injustice).

I just always recommend `point-min`.  Hard-coded constants are always
weird in source code, whereas `point-min` clearly says what this
constant is (and in terms of efficiency, it's a wash, or `point-min`
might even be marginally more efficient).


        Stefan

Re: [Emacs-diffs] master 75d9a55: Fix bug 32543

[Tino Calancha]

On Tue, 18 Sep 2018, Stefan Monnier wrote:

>>> - why store this info in text-properties rather than in
>>>   buffer-local variables?
>> I realized the buffer was already saved there with property
>> `occur-title'; I didn't wanted to scatter around related info.
>
> Hmmm... do you happen to have some intuition about why the buffer was
> saved in a text-property rather than in a buffer-local var?
> It's a really odd choice (less efficient, more work for the coder, with
> various side-problems like "on which char should I put it", etc...).
I think the favourite answer of my ex-boss fits well here:
For historical reasons.
> [ Also, why is it saved under a name like `occur-title` rather
>  than, say `occur-buffer`?  ]
Ditto.

>>>> +      (with-current-buffer buffer
>>>> +        (when (wholenump orig-line)
>>>> +          (goto-char 1)
>>>
>>> I'd recommend `point-min` instead of 1 here.
>> OK.  I always remember the discussion at
>> https://lists.gnu.org/archive/html/emacs-devel/2009-08/msg00520.html
>> but I forgot which was the encouraged practice there: in these cases
>> I follow the 50% rule to reject 0% success ratio (I know, it brings
>> 50% failure ratio: life is full of injustice).
>
> I just always recommend `point-min`.  Hard-coded constants are always
> weird in source code, whereas `point-min` clearly says what this
> constant is (and in terms of efficiency, it's a wash, or `point-min`
> might even be marginally more efficient).
I have a private library to autocomplete `point-min' (not just that!);
I just type
(pmin) <Super> a
and I get
(point-min)

During prototyping I sometimes write things like:
(goto-char 1)

I replace those `1' with `point-min' once I get
something ready.  I forgot this time.

Re: [Emacs-diffs] master 75d9a55: Fix bug 32543

[Stefan Monnier]

>> Hmmm... do you happen to have some intuition about why the buffer was
>> saved in a text-property rather than in a buffer-local var?
>> It's a really odd choice (less efficient, more work for the coder, with
>> various side-problems like "on which char should I put it", etc...).
> I think the favourite answer of my ex-boss fits well here:
> For historical reasons.

Ah, thanks, that explains it.


        Stefan