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

Simplify purescript-ds-create-imenu-index with hashtable #27

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

Hi-Angel
Copy link
Contributor

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

The function has implemented a poor man's hash table by manually enlisting keys and then jumping through the hoops by fetching them from one place, converting values to symbols to values… In particular, it was using symbol-value to map a symbol to its value, however the symbol was a local variable, and symbol-value doesn't work with them under lexical-binding, which the file was converted to since commit 9a9f550.

So fix the regression and simplify the code at the same time.

Fixes: #25

@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Nov 5, 2024

…oh, and I've grepped for other usages of symbol-value, it seems the only one that was referring to local symbols, so we should be fine in that regard.

Copy link

@niklas niklas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified to work for me (TM).

Hi-Angel added a commit to Hi-Angel/haskell-mode that referenced this pull request 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 Hi-Angel changed the title Migrate purescript-ds-create-imenu-index to hashtables Simplify purescript-ds-create-imenu-index with hashtable Nov 7, 2024
Hi-Angel added a commit to Hi-Angel/haskell-mode that referenced this pull request Nov 11, 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 added a commit to Hi-Angel/haskell-mode that referenced this pull request Nov 11, 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

UPD: changed for compatibility with older Emacs versions per discussion here

@Hi-Angel
Copy link
Contributor Author

Hi-Angel commented Nov 11, 2024

Well, and now it fails to compile on snapshot due to "obsolete macro"…

The function has implemented a poor man's hash table by manually
enlisting keys and then jumping through the hoops by fetching them
from one place, converting values to symbols to values… In particular,
it was using `symbol-value` to map a symbol to its value, however the
symbol was a local variable, and `symbol-value` doesn't work with them
under lexical-binding, which the file was converted to since commit
9a9f550.

So fix the regression and simplify the code at the same time.

Fixes: purescript-emacs#25
@Hi-Angel
Copy link
Contributor Author

Okay, I added a wrapper macro to silence the warning, it builds now on snapshot without warnings.

@purcell purcell merged commit 421fb28 into purescript-emacs:master Dec 9, 2024
10 checks passed
@purcell
Copy link
Member

purcell commented Dec 9, 2024

💪 thanks, merged.

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.

lexical-binding breaks decl-scan
3 participants