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.