Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Stefan Monnier]

> +   (let ((mark (mark-marker)))
> +     (and mark (marker-position mark) (copy-marker mark)))

Here you consider that (mark-marker) can return nil.

> +(defun save-mark-and-excursion--restore (saved-mark-info)
> +  (let ((saved-mark (car saved-mark-info))
> +        (omark (marker-position (mark-marker)))

But here you assume it's always a marker.

> +      (setf nmark (marker-position saved-mark))
          ^^^^
Old habits die hard.

> +    (let ((cur-mark-active mark-active))
> +      (setf mark-active saved-mark-active)
> +      ;; If mark is active now, and either was not active or was at a
> +      ;; different place, run the activate hook.
> +      (if saved-mark-active
> +          (unless (eq omark nmark)
> +            (run-hooks 'activate-mark-hook))

IIUC activate-mark-hook should also be run when (eq omark nmark) but
cur-mark-active was nil.


        Stefan

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Daniel Colascione]

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

On 05/04/2015 03:16 PM, Stefan Monnier wrote:
>> +   (let ((mark (mark-marker)))
>> +     (and mark (marker-position mark) (copy-marker mark)))
>
> Here you consider that (mark-marker) can return nil.
>
>> +(defun save-mark-and-excursion--restore (saved-mark-info)
>> +  (let ((saved-mark (car saved-mark-info))
>> +        (omark (marker-position (mark-marker)))
>
> But here you assume it's always a marker.

It is always a marker, isn't it? Some other parts of simple.el assume it
always returns non-nil. I'll remove the nil check.

>> +    (let ((cur-mark-active mark-active))
>> +      (setf mark-active saved-mark-active)
>> +      ;; If mark is active now, and either was not active or was at a
>> +      ;; different place, run the activate hook.
>> +      (if saved-mark-active
>> +          (unless (eq omark nmark)
>> +            (run-hooks 'activate-mark-hook))
>
> IIUC activate-mark-hook should also be run when (eq omark nmark) but
> cur-mark-active was nil.

Huh. I just mechanically translated the original save-excursion C code.
Doesn't that code have the same mismatch between the comment and the
behavior?

  /* If mark is active now, and either was not active
     or was at a different place, run the activate hook.  */
  tem = XSAVE_OBJECT (info, 3); // saved-mark-active
  ...
  if (! NILP (tem))
    {
      if (! EQ (omark, nmark))
	run_hook (intern ("activate-mark-hook"));
    }
  /* If mark has ceased to be active, run deactivate hook.  */
  else if (! NILP (tem1))
    run_hook (intern ("deactivate-mark-hook"));

I'll change the code to match the comment.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Stefan Monnier]

> It is always a marker, isn't it?

I think so, yes.

> Some other parts of simple.el assume it always returns non-nil.
> I'll remove the nil check.

Good, thanks.

>> IIUC activate-mark-hook should also be run when (eq omark nmark) but
>> cur-mark-active was nil.
> Huh.  I just mechanically translated the original save-excursion C code.

Ah, you're probably right: the save-excursion code had such kinds of problems.

> I'll change the code to match the comment.

Thanks,


        Stefan

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Nicolas Petton]

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


Stefan Monnier writes:

>> +      (setf nmark (marker-position saved-mark))
>           ^^^^
> Old habits die hard.

Should `setf' be avoided?  Is it only when `setq' would do instead?


Nico, confused
-- 
Nicolas Petton
http://nicolas-petton.fr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Daniel Colascione]

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

On 05/04/2015 04:02 PM, Nicolas Petton wrote:
> 
> Stefan Monnier writes:
> 
>>> +      (setf nmark (marker-position saved-mark))
>>           ^^^^
>> Old habits die hard.
> 
> Should `setf' be avoided?  Is it only when `setq' would do instead?

I'm guessing Stefan doesn't want loading simple.el (in the case where
it's not already compiled) to force the autoloading of gv.el.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Stefan Monnier]

>>> +      (setf nmark (marker-position saved-mark))
>> ^^^^
>> Old habits die hard.
> Should `setf' be avoided?  Is it only when `setq' would do instead?

Sorry, I guess `setf' work as well, here.  I've just been bitten by
bootstrapping problems too many times, but simple.el should be late
enough that it can't be a problem.


        Stefan

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Daniel Colascione]

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

On 05/05/2015 10:50 AM, Stefan Monnier wrote:
>>>> +      (setf nmark (marker-position saved-mark))
>>> ^^^^
>>> Old habits die hard.
>> Should `setf' be avoided?  Is it only when `setq' would do instead?
> 
> Sorry, I guess `setf' work as well, here.  I've just been bitten by
> bootstrapping problems too many times, but simple.el should be late
> enough that it can't be a problem.

There ought to be a file-local variable marking early-loadup files that
tells the compiler to warn about certain otherwise-safe facilities.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Stefan Monnier]

> There ought to be a file-local variable marking early-loadup files that
> tells the compiler to warn about certain otherwise-safe facilities.

IWBNI, but at the same time, it's pretty difficult to clearly
characterize that which is safe and that which isn't (and our bootstrap
is pretty close to the edge at various places, so the details do
matter).

I'm not sure it's really worth investing much effort into it.


        Stefan "who'd rather aim for a different bootstrap method to get
                rid of the prooblem"

Re: [Emacs-diffs] master 255a011: Add `save-mark-and-excursion', which has the old `save-excursion' behavior

[Nicolas Petton]

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


Stefan Monnier writes:

>>>> +      (setf nmark (marker-position saved-mark))
>>> ^^^^
>>> Old habits die hard.
>> Should `setf' be avoided?  Is it only when `setq' would do instead?
>
> Sorry, I guess `setf' work as well, here.  I've just been bitten by
> bootstrapping problems too many times, but simple.el should be late
> enough that it can't be a problem.

Ok, thanks for the clarification.

Nico
-- 
Nicolas Petton
http://nicolas-petton.fr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]