Re: Re-including rst.el into Emacs repository

[Stefan Merten]

Hi StefanMo and all!

3 weeks (21 days) ago Stefan Monnier wrote:
>> Just committed.  I'd appreciate a thorough review.
> 
> See some comments below (the patch is much too large to review in
> detail, and I trust you on the meat of the code, concentrating on
> nitpicks instead).

Thanks for the nitpicks. I implemented nearly all of your suggestions.

>>> But please try to keep some reference to the
>>> "common ancestor" and "tip" of the branch being merged (that's done
>>> automatically as Bzr metadata when it's a normal merge, but I suspect in
>>> your case the branch from which you merge is external).
>> I did this in the log message including a reference to the Docutils
>> subversion revision.
> 
> Thanks, tho you only put the tip of the merge, and forgot its base.

I don't understand. What do you mean by the base of the merge?

> If you check the ChangeLog guidelines, they recommend the use of the
> present tense: describe what the change does.
[...]
> Additionally, try and use the active
> form

Done. Please note that I'm German. We *love* the passive form over
here ;-) .

>> +  (let (rst-re-alist)
>> +    (dolist (re rst-re-alist-def)
>> +      (setq rst-re-alist
>> +        (nconc rst-re-alist
>> +               (list (list (car re) (apply 'rst-re (cdr re)))))))
>> +    rst-re-alist)
>
> Not that it matters, but this is an O(N^2) algorithm which you can turn
> into a O(N) algorithm by using (push (list (car re) (apply 'rst-re (cdr
> re)))) rst-re-alist) and a final nreverse.
> That's a classic "Elisp code pattern".

That doesn't work (unit tests fail). This whole thing is not very nice
because of the cyclic dependency between defining the constant and
`rst-re' - which also annoys the byte compiler... This will vanish
anyway when `sregex` or `rx` is used at some point.


                                                Grüße

                                                Stefan

pgprNJ0MoM4IO.pgp
Description: PGP signature