vedang / pdf-tools

Emacs support library for PDF files.
https://pdftools.wiki
GNU General Public License v3.0
643 stars 90 forks source link

Fix misuse of cl-ecase #159

Closed cadadr closed 2 years ago

cadadr commented 2 years ago

The cl-ecase macro does not support default cases (‘otherwise’ or ‘t’), and the cl-case macro requires the default case to be the last clause, which a recent change to Emacs rendered a macro expansion error, thus preventing the module from loading (and potentially signalling error in the init process).

Furthermore, cl-ecase expands to a cl-case form with a trailing special flag clause, so the appearance of a ‘t’ or ‘otherwise’ clause in a cl-ecase call will always result in this error being triggered.

I thus replace the apparently futile call to cl-ecase with an equivalent null check and a call to cl-case, which fixes both issues. I replaced cl-ecase with cl-case because the former form had both a t-check and a nil-check, which meant the error case was impossible to be triggered.

I wasn't able to run the tests, but I tested annotating a PDF with this change active and it worked alright.

marsam commented 2 years ago

Duplicate of #154

cadadr commented 2 years ago

Yeah indeed, sorry for not searching. Didn't think anybody else would've ran into this, for some reason.

My patch seems to do what @vedang says (and what the newer, unreleased version of cl-(e)case requires). FWIW I also sent an email to bugs-gnu-emacs, asking if they can add the relevant change to NEWS and also maybe turn it into a warning rather than a full load-time error.

cc @ebellani

vedang commented 2 years ago

Thanks, LGTM. Merging this change in

ebellani commented 2 years ago

hey, cool! Great job @cadadr

cadadr commented 2 years ago

thank you all!