bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module

[Eli Zaretskii]

> Date: Tue, 11 Apr 2023 00:02:03 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
>    module M
>      module N
>        module C
>          class D
>            def C.foo
>              _
>            end
>          end
>        end
>      end
>    end
> 
> (ruby-add-log-current-method) currently returns "M::C.foo"
> 
> While it should return "M::N::C.foo". Patch attached.
> 
> This discovery stems from Mattias Engdegård's report (in private) about 
> an ignored return value from `nreverse`.
> 
> Is this good for emacs-29?

I guess so.  But I wonder: this code was there since ruby-mode.el was
added to Emacs in 2008, so are you saying that (cdr ml) can never be
nil at this place, or if it is, it's okay to leave ml at the nil
value?

IOW, the return value of nreverse has been ignored, but what about the
nreverse call before that:

> --- a/lisp/progmodes/ruby-mode.el
> +++ b/lisp/progmodes/ruby-mode.el
> @@ -1911,7 +1911,7 @@ ruby-add-log-current-method
>                          (while ml
>                            (if (string-equal (car ml) (car mn))
>                                (setq mlist (nreverse (cdr ml)) ml nil))
> -                          (or (setq ml (cdr ml)) (nreverse mlist))))
> +                          (setq ml (cdr ml))))
>                        (if mlist
>                            (setcdr (last mlist) (butlast mn))
>                          (setq mlist (butlast mn))))

Isn't the second nreverse there to undo the first?

I guess I'm asking for slightly more detailed rationale for the change
you are proposing, to include the analysis of the code involved and
where that code is mistaken.  For example, why not say

   (setq mlist (nreverse mlist))
or
   (setq ml (nreverse mlist))

instead of just removing the 2nd nreverse call?

Thanks.