Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize and simplify haskell-ds-create-imenu-index with hashtable #1857

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

Hi-Angel
Copy link
Contributor

@Hi-Angel Hi-Angel commented Nov 7, 2024

The function has implemented a poor man's hash table by manually enlisting keys and then using cl-case on values. Just replace this with a hashtable and thus simplify the code considerably. This makes it faster as well because the cl-case + setq is O(n) whereas puthash is O(1).

The output of the function was tested on Pandoc's
Writers/Powerpoint/Output.hs file of 2841 lines and comparing the hashsum of the output before the changes and after. It didn't change.

The commit is also basically a backport of a similar change on purescript-mode that has been forked form haskell-mode at some point: purescript-emacs/purescript-mode#27

@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Nov 7, 2024

Just in case: CC: @cocreature as the last person who touched the function

@purcell
Copy link
Member

purcell commented Nov 11, 2024

Thanks for this. I approved the CI and it's complaining about part of the change, not immediately obvious to me why. :)

@Hi-Angel
Copy link
Contributor Author

Hmm, bizarre indeed… The snippet (curr-alist (gethash (car type) imenu)) (as well as the line, and the whole "added" side of the diff for that matter as far as I compare) is exactly the same as in the purescript-mode PR, however there byte-compilation passed just fine everywhere starting with 25.1. Very strange.

Okay, thank you, I'll look at it later

@purcell
Copy link
Member

purcell commented Nov 11, 2024

It's a warning that is promoted to an error, but the cause is when-let* not necessarily being defined. Adding (require 'subr-x) fixes it for me, at least in Emacs 28.x.

@Hi-Angel Hi-Angel force-pushed the hashtable-optimization branch from b95296a to 89c6f2d Compare November 11, 2024 13:56
@Hi-Angel
Copy link
Contributor Author

Oh, thank you! I was comparing the require lines on the purescript and haskell "decl-scan" files, but they are the same, so I dismissed that thought. But perhaps in "purescript-mode" the (require 'subr-x) line comes from another import (both haskell and purescript "decl-scan" files include the main mode file).

I added the line, let's see if CI will be okay with it

@Hi-Angel
Copy link
Contributor Author

Welp… 😊 I'll dig into it later, don't bother for now, thank you!

@purcell
Copy link
Member

purcell commented Nov 11, 2024

But perhaps in "purescript-mode" the (require 'subr-x) line comes from another import (both haskell and purescript "decl-scan" files include the main mode file).

Yeah, I don't really like that pattern of importing, partly for this reason!

The error with Emacs 25.1 is because when-let* was only added to subr-x in Emacs 26.1. Probably best to avoid that macro — when-let was available in Emacs 25.1, so you can just use that instead.

@Hi-Angel
Copy link
Contributor Author

The error with Emacs 25.1 is because when-let* was only added to subr-x in Emacs 26.1. Probably best to avoid that macro — when-let was available in Emacs 25.1, so you can just use that instead.

Well, when-let* compiles just fine on 25.1 with the same change on purescript-mode

@purcell
Copy link
Member

purcell commented Nov 11, 2024

Huh! 🤔

@Hi-Angel
Copy link
Contributor Author

Well, when-let* compiles just fine on 25.1 with the same change on purescript-mode

Oh, actually, scratch that. I just looked at the job log, and apparently the purescript-mode CI, for some reason, doesn't compile that specific file at all. It compiles other files, but not this one. So most likely this is the reason I don't see the failure there.

The function has implemented a poor man's hash table by manually
enlisting keys and then using `cl-case` on values. Just replace this
with a hashtable and thus simplify the code considerably. This makes
it faster as well because the `cl-case + setq` is O(n) whereas
`puthash` is O(1).

The output of the function was tested on Pandoc's
`Writers/Powerpoint/Output.hs` file of 2841 lines and comparing the
hashsum of the output before the changes and after. It didn't change.

The commit is also basically a backport of a similar change on
purescript-mode that has been forked form haskell-mode at some point:
purescript-emacs/purescript-mode#27
@Hi-Angel Hi-Angel force-pushed the hashtable-optimization branch from 89c6f2d to a4e246e Compare November 11, 2024 14:57
@Hi-Angel
Copy link
Contributor Author

Okay, in this case, another attempt… I changed when-let* to when-let 😊

The reason I attempted when-let* is because when-let was declared obsolete in preference of when-let* in current Emacs master…

@purcell
Copy link
Member

purcell commented Nov 11, 2024

The reason I attempted when-let* is because when-let was declared obsolete in preference of when-let* in current Emacs master…

Yeah, always tricky juggling these things when aiming for compatibility with a wide range of Emacsen. It's possible that the compat package helps here, but I'd rather not add it as a dependency just for this case.

Merging now, thanks!

@purcell purcell merged commit 1a285fc into haskell:master Nov 11, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants