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

Make hash-table support numbers as keys #431

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

nagy
Copy link
Contributor

@nagy nagy commented Mar 14, 2022

This allows numbers to be used as hash-table keys, since they are crudely converted to strings.

Closes #424

@vlad-km
Copy link
Member

vlad-km commented Mar 29, 2022

@nagy, thanks.
Sorry, very busy lately.
I'll take a look at the code this weekend.

@davazp
Copy link
Member

davazp commented Mar 29, 2022

I think a requirement for this change is that (gethash 1 ht) should be different from (gethash "1" ht), of even for JS objects.

For #'eq hash tables, we could just probably use a Map.

But that doen't solve the problem for #'eql hash-tables.

@nagy
Copy link
Contributor Author

nagy commented Apr 1, 2022

that (gethash 1 ht) should be different from (gethash "1" ht)

You are right, I will think of a different approach.

@nagy nagy marked this pull request as draft April 1, 2022 20:18
@hemml
Copy link
Contributor

hemml commented Apr 1, 2022

May be we can store keys as strings like "TYPE:VALUE", for example "1" will be converted to "string:1" and 1 will be "number:1", etc.

With this change, numbers are now treated as strings when setting
properties of javascript objects. This aligns with how object properties
are accessed in javascript already:

```sh
$ node
Welcome to Node.js v18.7.0.
Type ".help" for more information.
> var o = {}
undefined
> o[111] = 222
222
> o["111"]
222
>
```

Before this, the following error would be thrown.

```
CL-USER> (jscl::oset 222 (jscl::new) 111)
ERROR: activechars.join is not a function
CL-USER>
```
@nagy nagy marked this pull request as ready for review August 22, 2022 21:16
@nagy
Copy link
Contributor Author

nagy commented Aug 22, 2022

To my surprise, that distinction between numbers and strings is already happening correctly. I have split the commit into two and added some more test and wrote some reasoning into the commit message.

Please check if it is fine now.

@davazp davazp merged commit 29885f7 into jscl-project:master Aug 23, 2022
@davazp
Copy link
Member

davazp commented Aug 23, 2022

Awesome! Thanks!

@nagy nagy deleted the numbers-as-hash-keys branch August 23, 2022 10:52
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.

EQ-HASH refactoring
4 participants