Re: Proposal of a better hash table implementation based on SRFI 125

[Amirouche Boubekki <amirouche.boubekki@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5776 bytes --]

Thanks!

I have done mostly a cosmetic review of your code. Here is the summary:

- There is two big procedures that I think should be split `
get-hash-functions` and `hash-table` to help readability
- I am not very fond of  generic-hash-table macros, I would rather use
apply / call-with-values, if there is an optimisation to be done it should
be done in the compiler... no?

Otherwise:

- You follow the GNU guidelines for commit message that is a good thing
(minor niptick, you should use present tense "Fix" instead of "Fixed",
"Implement" instead of "Implemented"
- The code is very readable

I don't know about whether that code requires additions to the manual since
it's R7RS and SRFI.

I cc'ed Mark Weaver.

Le jeu. 17 janv. 2019 à 22:14, Jéssica Milaré <jessymilare@gmail.com> a
écrit :

> *Note
>
> Em qui, 17 de jan de 2019 às 19:13, Jéssica Milaré <jessymilare@gmail.com>
> escreveu:
>
>> Done.
>>
>> Not that there are many commits. The first commit only fixes bug 33827 of
>> SRFI 69, and SRFI 69 is reimplemented in the third commit, which pretty
>> much erases the changes of the first commit.
>>
>> https://github.com/jessymilare/guile/pull/1
>>
>> Em qua, 16 de jan de 2019 às 22:19, Amirouche Boubekki <
>> amirouche.boubekki@gmail.com> escreveu:
>>
>>> I will try to look at the commits. One last niptick, can you push guile
>>> master branch to you repository, and make a pull request against it so that
>>> I can directly comment in the pull requests. please. It's easier to way to
>>> see the overall changes in particular.
>>>
>>> Le dim. 13 janv. 2019 à 23:52, Jéssica Milaré <jessymilare@gmail.com> a
>>> écrit :
>>>
>>>> Finally finished the libraries.
>>>>
>>>> SRFI-125 is implemented. I took the liberty to create a procedure
>>>> scm_hash_n_items in 'libguile/hashtab.c' that works with both normal and
>>>> weak hash tables, so the GENERIC-HASH-TABLES module don't need to keep
>>>> track of the hash table size anymore.
>>>>
>>>> `make check` runs everything ok. I believe it's ready for testing. Any
>>>> feedback is welcome.
>>>>
>>>> Regards,
>>>> Jéssica
>>>>
>>>> Em sáb, 12 de jan de 2019 às 18:34, Jéssica Milaré <
>>>> jessymilare@gmail.com> escreveu:
>>>>
>>>>> (It seems I mistakenly responded only to a personal e-mail, sorry,
>>>>> responded now to everyone in the list with updates).
>>>>>
>>>>> So here are the news:
>>>>>
>>>>> I've created a module called (ice-9 generic-hash-tables), which is
>>>>> similar to SRFI-125, and used it to implement SRFI-69, (RNRS HASHTABLES),
>>>>> SRFI-126 and SRFI-125. Since SRFI-125 depends on SRFI-128, I've implemented
>>>>> it as well. My public repository already has SRFI-128, I'm just finishing
>>>>> some tests with SRFI-125 and, once they are done, I'll push it as well.
>>>>>
>>>>> The module (ice-9 generic-hash-tables), is quite usable and all
>>>>> exported procedures are documented. I've created tests for it and also
>>>>> ported standard tests from SRFI-126.
>>>>>
>>>>> Now, I see that 'libguile/hashtab.h' code keeps track of the number of
>>>>> elements in hash tables, but the field is not visible from Scheme. Can that
>>>>> be changed? Then generic hash tables wouldn't need to also keep track of
>>>>> the number of elements (like SRFI-69 currently does) and that would
>>>>> simplify its code.
>>>>>
>>>>> Besides, what about creating new versions HASHX-* procedures that
>>>>> accept an equivalence procedure instead of an assoc procedure? Perhaps
>>>>> prefixed by 'NEW-' or post-fixed by a '*'.
>>>>>
>>>>> I've also found inconsistencies between SRFI-125 and it's reference
>>>>> implementation and standard tests. I've implemented according to the
>>>>> specification - so, for instance, HASH-TABLE=? checks if the equivalence
>>>>> function of both hash tables are the same and HASH-TABLE returns an
>>>>> immutable hash table.
>>>>>
>>>>> Code is public and suggestions are always welcome :)
>>>>> https://github.com/jessymilare/guile
>>>>>
>>>>> Regargs,
>>>>> Jéssica
>>>>>
>>>>> Em dom, 30 de dez de 2018 às 15:50, Amirouche Boubekki <
>>>>> amirouche@hypermove.net> escreveu:
>>>>>
>>>>>> Le 2018-12-28 17:11, Jéssica Milaré a écrit :
>>>>>> > Hello,
>>>>>> >
>>>>>> > As I said in a previous e-mail, currently SRFI-69 is broken for weak
>>>>>> > hash tables - and I've sent a patch to fix it. However, I think
>>>>>> there
>>>>>> > are many other problems with current implementation of hash tables.
>>>>>> > There are guile standard hash tables, SRFI-69 hash tables (which is
>>>>>> > implemented on top of standard hash tables) and also R6RS hash
>>>>>> tables
>>>>>> > (which is implemented on top or SRFI-69 and completely lacks support
>>>>>> > for weak keys and/or values).
>>>>>> >
>>>>>> > I think that should be fixed and guile should have only two kinds of
>>>>>> > hash tables: the standard guile hash table and another extended hash
>>>>>> > table type that will be used directly by R6RS, SRFI-125 and SRFI-69.
>>>>>> > In my opinion, it should be based on SRFI-125, which is part of R7RS
>>>>>> > Red Edition, but also supports some other procedures to make it
>>>>>> > compatible with R6RS and SRFI-69, supporting weakness and immutable
>>>>>> > hash tables.
>>>>>> >
>>>>>> > I'm already implementing the SRFI-125 based hash tables library for
>>>>>> > myself, so, if that is accepted, I can also make a patch for guile.
>>>>>> >
>>>>>>
>>>>>> Yes please make a patch and attach it to the bug report.
>>>>>>
>>>>>> Don't forget to send an announcement when you SRFI-125 implementation
>>>>>> is ready for testing. Is it already available anywhere?
>>>>>>
>>>>>> TIA
>>>>>>
>>>>>

[-- Attachment #2: Type: text/html, Size: 8128 bytes --]