Re: [rt.cpan.org #125472] error with coderefs passes as scalar args

Also cc'd  to [email protected] 
https://www.nntp.perl.org/group/perl.tcltk/2018/07/msg718.html
per Vadim's request


At 09:07 AM 7/3/2018, you wrote:
><URL: https://rt.cpan.org/Ticket/Display.html?id=125472 >
>
>
>Thank you for the prompt and detailed response.
>
> > I added comments to the tests i added to t/ to document the problems
> > i noticed with the v1.05 process that you may find helpful to read.
> >
> > I found that others were already using the create_tcl_sub() entry
> > even though it was not documented. And i found that the description
> > of CreateCommnd() to be confusing to me as well.
>
>I think someone uses the internal function does not necessary mean 
>that the function has to be exposed by document. The difference is: 
>If a module user uses undocumented internal function, he/she takes 
>the risk of function robustness and compatibility on module 
>revision. On the other hand, if a module document a function as an 
>API, the module shall take response that both interface and behavior 
>should not change in the future.


I agree that documented interfaces should continue to work as 
documented, but that does not mean they must stay static, it just 
means they should not bust any pervious uses.

After looking at tcl.pm/.xs it was clear to me that create_tcl_sub() 
should be the preferred method for introducing code to tcl because 
CreateCommand() was so vaguely documented and its arguments are very 
specific to the tcl interface and can cause bigger problems is used 
improperly.

create_tcl_sub() takes care of much of the needed background 
processing needed to properly create a tcl<=>perl subroutine such as 
placeing it into the proper namespace and returing the tcl name in 
that namespace. it also populates the tracking structure such that 
further uses of a static coderef will not repopulate the tcl 
subroutine internal reference (in v1.02- and  v1.06+ at least) if it 
is found a second time by call().


> > Since create_tcl_sub() is what was used in call(), and that was that
> > pair where the code needed to be patched i thought it made sense to
> > document how create_tcl_sub() worked and how code disposal worked
> > within it. IMHO it made sense to expose create_tcl_sub() so that
> > programmers could use the automatic recoding of a call(,,$coderef)
> > process as well as being able to control when code disposal took
> > place if they wanted to handle it themselves. This allows them to
> > continue to use coderefs if they want and just delete one use by
> > using DESCRNAME, or all clear it totally by using TCLNAME.
>
>Yes, it makes sense to document how internal mechanism works, but 
>again it does not mean create_tcl_sub() has to be documented so 
>detail and as a part of API.
>
> > I think i made it clear that the user supplied DESCRNAME can be
> > anything they want it to be, as long as it does not begin with '='
> > or '@', or '::perl::'.
> > '=' now prefaces "normal" entries created by create_tcl_sub().
> > '@' prefaces entries create_tcl_sub() will use to possibly dispose of
> > coderefs "AT" a future time since they were used by an after command.
> > and '::perl::' prefaces the TCLNAME assigned by create_tcl_sub() and
> > is used in anon_refs{} to determine when no more "users" exist and
> > the coderef can now be safely deleted from the tcl namespace.
>
>Okay, I finally get the the point of DESCRNAME usage now. I did not 
>know that DESCRNAME can be passed to _code_dispose() so I did not 
>get the purpose of DESCRNAME at all. I suggest to have more explicit 
>description that argument of _code_dispose() is either TCLNAME or 
>DESCRNAME. And the major purpose of DESCRNAME is used to pass to 
>_code_disposal().
>I get your point more, but I still think we shall properly define 
>the interface to support code disposal instead of just directly 
>expose an internal function out. For example, how about modify 
>call() to support the form of call(DESCRNAME, cmd, arg ...), where 
>DESCRNAME is optional and is a string that with '@' prefix. Then we 
>have a good compatibility and user do not have to worry about 
>GENCODE, EVENTS etc.


The main purpose of DESCRNAME is to name the slot used in $anon_refs 
to store information about the tcl<=>perl subroutine linkage and 
track it. Just one of the purposes of tracking is to be able to 
dispose of it later.

I do not understand why you mean to accomplish by modifying call() 
that way, how would call understand that it is a descrname and not a tcl verb?

The @ prefix was selected to identify a group of coderefs that 
might  be able to be deleted by a call() generated "after" call to 
_code_dispose once the initiating "after" call has completed.  For 
the most part $anon_refs keys beginning with '@" should be very short 
lived transients. I think adding another use for the '@' prefix would 
be counterproductive. And a call() can also have more than one 
coderef in its args as call('if','1',$sub1,$sub2) illustrates.

The documentation for Tcl::_code_destroy(NAME) already states
 >calling _code_destroy on a TCLNAME retruned from create_tcl_sub 
removes all use instances and purges the command table.
 >calling _code_destroy on a DESCRNAME passed to create_tcl_sub 
removes only that instace
 >Code used in a DESCRNAME may be used in other places as well,
 >only when the last usage is purged does the entry get purged from 
the command table

I think that is a pretty good description of its modes, but if you 
can suggest a better description you are free to have a go at it.

I can understand why you think that EVENTS should be hidden better, 
and then GENCODE as well, but both of them were well integrated into 
the existing code and i did not want to break that.  I can also see 
cases where a user may want to use the EVENTS parm tho, and in that 
case they would want both the TCLNAME and GENCODE returned, so i 
introduced a change to what was returned to allow for that. Existing 
code expecting a scalar to be returned still gets GENCODE as it did 
before, but new code expecting an array can get both of them.  And 
uless EVENTS was populated both are the same anyway.

> > > 2) The newest POD in github HEAD mentioned
> > >
> > > $interp->call('fileevent','sock9827430','writable');
> > >
> > > won't dispose the sub created from
> > >
> > > $interp->call('fileevent','sock9827430', 'writable'=>sub{...});
> > >
> > > is it still true? Or only 'set' command behaves in this way?
> >
> > yes this is still true, it was true in v1.02 and v1.05 and reverted
> > back to that behavior in v1.11. at v1.06 i had tried to make it so
> > the first call would cause code disposal of the sub created by the
> > second, but that was what broke your "set foo" tests. At the time i
> > introduced that behavior i thought it made sense to try to initiate
> > code disposal in those cases, to keep the internal table and tcl
> > namespace clean, but after spending some time trying to decide if
> > "set foo" was the only place this should be avoided i decided that
> > it was safer to revert to the original processing where code
> > disposal would only be triggered if there was a different coderef in
> > the new call. but not when the coderef was now missing.
> >
> > I wrapped the place where it occurs with a flag so it could be
> > reinstated if desired when it was determined that only "set foo"
> > should be handled in this special manner
>
>Too bad that it is disabled by default. IMO 
>'$interp->call("FILEEVENT" ... "WRITABLE=>"")' shall be a suggested 
>way to dispose the code and well documented. 'set' command is an 
>exception and to be documented. Of course there are other exceptions 
>to be found out. The flag SAVENOCODE might be better documented in POD too.

i did not document SAVENOCODE nor SAVEALLCODES because i feel both to 
be transient flags destined to be deleted when the tcl.pm code is solidified.

both serve to temporarily encapsulate code i felt to be useful but 
might not deserve to be in the final code, and allow the encapsulated 
code to be toggled on/off to see how it works in different 
situations.  If you look at where both are used i think you will find 
that i did document what i was trying to do when i introduced the 
code they encapsulate. Again if you think you can explain it better 
please feel free to do so.

i am not a fan of  "Of course there are other exceptions to be found 
out." methods. It disgusted me greatly that my introduction of the 
code to deal with '$interp->call("FILEEVENT" ... "WRITABLE=>"")' (or 
its bind cousins) busted the "set  foo"  test in Tkx.pm.  I hate to 
bust existing working code and feel it diminishes my reputation as a 
responsible programmer when i do so.

I would rather shut it off for now, reverting to the existing process 
in v1.02/v1.05 than to play catchup to bug reports stating it busted 
something else. I have been thru much of the tcl/tk documentation 
looking for other cases the newer behavior may also break but so far 
have only identified the "set foo" instance, but that does not mean 
no other cases still exist and would rather revert to the previous 
behavior until i can be assured that the "set foo" instance is the 
only one to be exempted rather than bust someone elses code and 
require them to figure out how to file a bug report about it.   When 
we are assured that the code will do what we want it is a simple 
matter to remove the encapsulation and delete the transient flags.

>SJ

This new code disposal process is opening a can of worms, but that 
does not mean it cannot be accomplished successfully. As i was 
working on the reference counting i imagined inserting a new code 
disposal method that could clean up after a deleted tk element, 
searching the keys of %anon_refs for  keys prefixed with '=' 
(internally generated)  and tracking button/bind/menu objects related 
to a element name specified as a parameter. It could then dispose of 
them in the normal method, freeing any that are no longer referenced. 
while i did not think i should being this up until the code disposal 
code has stabilized more, i think now is not a bad time to mention it 
as it serves to solidify the direction i think code_disposal should 
proceed in.