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

Updates for Tcl 9 and ensemble fixes. #46

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

hvxl
Copy link

@hvxl hvxl commented Sep 19, 2024

I made changes necessary to be able to compile the library against Tcl 9.0, while still also being able to compile against Tcl 8.6. Additionally I adapted the test suite to account for the differences when the library is compiled with --enable-ensemble.

@cyanogilvie
Copy link
Member

This looks great, thanks for this work. It's been on my list for months, but we use 8.7 in production and other work things have taken priority.

Broadly I'm happy to merge this, I'd just like to discuss a few of the details, which are hopefully added as comments to the diffs (I'm pretty new to github PR reviews, let me know if I've done something wrong or you can't see the comments). Mostly I'd like to align the rl_json commands and exported stubs functions with Tcl when it's in a Tcl 9 environment, so that the data structures it can work with have the same sizes (very long JSON arrays, etc.). I haven't moved any of my stuff to Tcl 9 yet, so I don't have practical hands-on experience with it yet, so I'd value your insights into the porting considerations, particularly whether it can be safely exposed to Tcl 8 (with size == int) and Tcl 9 (size == Tcl_Size), possibly with a polyfill #define Tcl_Size int when building against Tcl 8.

On the ensemble differences: I'm not really that married to keeping it around at all. With code as hot as some of these are (if it's being used like one would use dict, which we do a lot), then the little bit of extra overhead for the ensemble dispatch really adds up (sometimes nearly doubling the command cost). I'd be interested to benchmark it in Tcl 9 to see if that's still the case - I have many other extensions that have to make the same choice about how to implement an ensemble-style interface - but if it's still as costly as Tcl 8 I'd be in favour of just dropping it and cashing in the complexity saving. The only argument I can think of against that would be if people were using the fact that it was an actual ensemble to extend it with their own subcommands, but I'm not aware of anyone actually doing that.

@hvxl
Copy link
Author

hvxl commented Jan 28, 2025

Unfortunately I don't see your comments. I'm also not well versed in github PR reviews, so I can't say which one of us is doing something wrong. It may just as well be me.

I was not aware of the performance penalty that comes with using the ensemble version. In my application performance isn't all that important. In the past I did add a getdef subcommand, which is why I was using the ensemble. But now that a -default option has been added, that can be used instead. For backward compatibility I can just add an ensemble on top of the ensemble-less implementation.

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.

2 participants