Implement gensym #279
Implement gensym #279
Conversation
|
RFR @hylang/core |
|
Moved the gensym symbol to a UTF reserved char. Added tests. Ignoring threading. We have other race conditions all over the place too. If someone has a fix that won't dent perf hard, send in a PR |
|
RFR @hylang/core |
|
minor nitpick: gensym should be moved up, as @rwtolbert ordered it alphabetically. If you do, please add a note at the beggining advising others to keep it that way it is easy to miss. I'd prefer there was 2 different arguments for index number and full prefix instead of one that does both jobs depending on what one uses as an argument. I you still prefer to have just 1 arg. it should probably be explained in the docstring. If we are ignoring concurrency and the prefix is fixed it is fine for me, but I'm no hylang/@core so others must judge 😉 |
|
Looks good to me overall, except for the multithreading issue. Of course, the chance of concurrent invocations of gensym is extremely small, since in normal use it is called only the first time any module is imported. But when there is a race condition, the consequences are subtle and irreproducible bugs in the Hy program being run. So I propose protecting gensym by a lock. Performance shouldn't be a problem for anything run only at compile time. |
|
I changed my mind about the multithreading issue. We haven't cared about it elsewhere, so I propose we open an issue where we note all the global state that can potentially create trouble and find a global solution one day. Thread-local storage is perhaps a better solution than locks. |
|
It's going to be a PITA to look for multi-threading issues later. I don't see how it's hard to use a basic lock mechanism right now. We can replace it by something else later, but at least, we won't miss it. (with a test). |
|
I'm filing a new bug. This isn't the only race condition we have, I'll defer this until we have a chance to audit (but I'll make such a multithread audit blocking the next release) |
|
Rebased and took @Willyfrog's suggestion (Hey, thanks!) |
I'm not exactly sure if this is the best way to implement it, but I've gone and matched this implementation to the interface shown in: http://clhs.lisp.se/Body/f_gensym.htm Close hylang#278
Also add tests for gensym behavior.
|
I've filed the issue as #290 Mazel tov |
|
This PR is deprecated in favor of @rwtolbert 's #374 |
I'm not exactly sure if this is the best way to implement it,
but I've gone and matched this implementation to the interface
shown in:
http://clhs.lisp.se/Body/f_gensym.htm
Close #278