gisle / tcl.pm

Tcl extension module for Perl
http://search.cpan.org/dist/Tcl
9 stars 8 forks source link

Various POD and comments fixes #36

Closed chrstphrchvz closed 3 years ago

chrstphrchvz commented 3 years ago

Here are the remaining POD and comments fixes proposed by @marekro in https://rt.cpan.org/Ticket/Display.html?id=132735 which have not already been addressed by others since the latest Tcl.pm release (1.27).

vadrer commented 3 years ago

thanks

marekro commented 3 years ago

Thank you for including the documentation fixes. However, the patch in the RT ticket also contained code to support tied ARRAYs (Perl) to TCL lists. I am not sure whether I chose the most appropriate package name - feel free to adapt it. Anyway, I feel that this could be a welcome enhancement of the Tcl package, such that TCL lists can be used like a Perl array, transparently...

chrstphrchvz commented 3 years ago

While I like the idea of tying Perl arrays and Tcl lists, I think it may require more discussion before implementing. I will probably leave comments regarding it soon. Not something typo fixes need to wait on, and they're probably better to commit separately from any more significant changes.

vadrer commented 3 years ago

I like it, actually. Although I do prefer to keep Tcl.pm as small as possible, and probably the proposed addition will be rarely used, however it is logical, well written and indeed perfectly fits the module

+    my ($obj, @list) = @_;
+    my ($interp, $varname, $flags) = @{$obj};

I prefer here just @$obj

+sub FETCH {
+    my $obj = shift;
+    unless(@{$obj} == 2 || @{$obj} == 3) {

here I do not really insist, because $#$obj==1 || $#$obj==2 contains about the same amount of characters; however it does not involve "context", so eye-parsed better (IMO)

chrstphrchvz commented 3 years ago

I think it would be better for the implementation to rely heavily on Tcl commands using the corresponding list variable, and avoid retrieving/splitting/recreating the whole list. Examples I can think of:

I think STORESIZE should not be a no-op: it should discard trailing elements or append empty elements as needed (lrepeat in Tcl 8.5+ sounds ideal, especially for anyone who wants to preallocate a large list). EXTEND will likely remain a no-op if there is no way to "invisibly" preallocate a list. I don't know what EXISTS and DELETE should do.

Another option is to use Tcl C API list functions from XS. But even with a Perl implementation in a new class, does anything in existing XS code need adjusting, e.g. TclObjFromSv()/SvFromTclObj()?

chrstphrchvz commented 3 years ago

Also, although I currently cannot think of what I would use a Perl array tied to Tcl list for, maybe someone else does—and so this a "nice to have" feature for the module.

I am curious if the existing Tcl::List or Tcl::Var class could be extended/replaced, especially if doing so is compatible with any existing usage, or at least if sufficient warning of disruption is given (e.g. bump to version 2.00). Maybe other more radical API redesign simultaneously is desirable: something I might find more useful right away would be for GetVar() and other methods to automatically split lists when called in array context.

vadrer commented 3 years ago

I think it would be better for the implementation to rely heavily on Tcl commands using the corresponding list variable, and avoid retrieving/splitting/recreating the whole list. Examples I can think of:

  • FETCH should use lindex
  • STORE should use lset (or lreplace if pre-8.4 must remain supported)
  • PUSH should use lappend
  • SHIFT/UNSHIFT/POP should use lreplace
  • SPLICE should use lreplace and lappend
  • FETCHSIZE should use llength

yes I agree - @marekro 's implementation isn't the fastest, and I agree that most operations should be better done on Tcl side.

I think STORESIZE should not be a no-op: it should discard trailing elements or append empty elements as needed (lrepeat in Tcl 8.5+ sounds ideal, especially for anyone who wants to preallocate a large list). EXTEND will likely remain a no-op if there is no way to "invisibly" preallocate a list. I don't know what EXISTS and DELETE should do.

Another option is to use Tcl C API list functions from XS. But even with a Perl implementation in a new class, does anything in existing XS code need adjusting, e.g. TclObjFromSv()/SvFromTclObj()?

ok, this brings us to the question: if the feature is really needed - we could go in a way that you've just described.

otherwise - if some "quick" workaround is needed - we can provide current code, but with a some kind of documentation warning that this approach is not efficient, and to gain efficiency another approach is needed

marekro commented 3 years ago

I am perfectly OK with using TCL code (through XS or otherwise), my implementation wasn't meant to be performance optimal. I think it is a big plus of this Perl/Tcl hybrid (compared to Python's tkinter for example) that one can directly map variables and even procedures, and get return values directly. That allows to write hybrid programs; not that this was a goal, but sometimes it is a necessity. For example, when I came up with this, I have been working on some EDA/CAD code, which colleagues wrote in TCL (because some tools use TCL internally), but I wanted to get access to the variables they defined in TCL from Perl, to process them further. Hoewever, they relied on some other TCL variables being set, so I was happy I could

  1. intialize TCL variables from Perl
  2. source the TCL script(s)
  3. read variables from TCL, and/or call TCL procedures from Perl
  4. do further stuff in Perl with the data All that worked wonderfully; but then, I got TCL lists back from some procedures, and I thought that it would be cool to be able to tie the TCL return value to a "TCL List" array object, and access the TCL list in Perl as if it were a Perl array. And save others from having to do all the split() etc. heavy-lifting. I am afraid I won't be able to work on optimising the implementation along the XS/built-in TCL path, but I am not offended if someone else picks that up. And since Rome wasn't built in a day, I would be happy to see my humble contribution being added as is, until it will be optimized/refactored. And yes, adding a note on the (missing) performance optimisation is perfectly fine for me.
vadrer commented 3 years ago

all true can you please create a pull request - we'll merge it and release next Tcl version on CPAN.

yet IMO adding code into XS for efficiency isn't actually needed in your case, so pure-perl implementation is pretty good.