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

An associative containers in which both types can be used as key. #238

Open
bobeff opened this issue Jun 19, 2020 · 8 comments
Open

An associative containers in which both types can be used as key. #238

bobeff opened this issue Jun 19, 2020 · 8 comments

Comments

@bobeff
Copy link

bobeff commented Jun 19, 2020

In some situations there is a need for an associative container in which both types can be used as key, similar to Boost.Bimap for C++. I have such an implementation called BiTable for the project I'm working on and I'm wondering whether there is an interest for something similar in the standard library. If so and if the proposed implementation is considered good enough I will write documentation for it and I will make a pull request.

The interface is based on both Boost.Bimap (leftView and rightView procedures) and Nim's standard library tables module.

@ghost
Copy link

ghost commented Jun 19, 2020

I think @Araq already implemented a BiTable, forgot where it was though

@zedeus
Copy link

zedeus commented Jun 19, 2020

https://github.com/nim-lang/compilerdev/blob/master/compiler/bitabs.nim

@bobeff
Copy link
Author

bobeff commented Jun 19, 2020

@Yardanico @zedeus Unfortunately this container implementation is not generic enough for the most use cases. It seems like something written specifically for a use case inside Nim's compiler.

  • The one of the keys is always Id (unit32).
  • It doesn't have a complete interface.
  • It is put in the compiler internals, not in the standard library.

On a good side it is more efficient than mine which is just a thin wrapper around two HashSet containers, because by re-implementing a hash table, it doesn't require dynamic allocation for every key/value pair with which it operates in some way.

I don't know @Araq's intentions and whether he plans to make it more generic and to put it into the standard library, but if he has such plans it can be a better alternative than my version.

@timotheecour
Copy link
Member

timotheecour commented Jun 19, 2020

(this is not an argument against a generic bitable)
There are too many table (or table-like) implementations in nim repo (see #200 (comment) and nim-lang/Nim#13418 (comment)) in particular the ones that are used just for the compiler.

nim-lang/Nim#13418 (comment)

instead of optimizing N different implementations you "optimize" a single one for diverse use cases. But this makes the problem much harder to solve, optimization is specialization. For example cellsets hashes pointers, nothing else. The only reason so much effort has to be put into the stdlib's hash tables is that we cannot anticipate how it's used.

the counterpoint of optimization is specialization is premature optimization is the root of all evil. We need a performance benchmark to justify the existence of compiler-specific table-like implementations; if there's at least one area where performance improves by a specialized implementation, then fine; if not, then compiler should dogfood stdlib where appropriate and the specialized implementation should be removed and performance regressions tests should be used in testament. I suspect that most compiler specific custom table implementations are not making any positive performance impact.

@Araq
Copy link
Member

Araq commented Jun 19, 2020

The compiler's hash tables have no known bugs and we can refactor them whenever we want to. Or never. I don't understand why you're pushing for this.

@github-actions
Copy link

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 20, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 21, 2023
@Araq Araq reopened this Jul 21, 2023
@Araq
Copy link
Member

Araq commented Jul 21, 2023

No, we really need this.

@konsumlamm
Copy link

In the standard library? Why?

@github-actions github-actions bot removed the Stale label Jul 22, 2023
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

No branches or pull requests

5 participants