At 01:46 AM 7/7/2018, you wrote:
<URL: https://rt.cpan.org/Ticket/Display.html?id=125472 >
Hi,
> > 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.
Yes. But any change has possibility to bust previous uses. The more
unnecessary internal function exposing, the more compatibility issue
have to be considered and the more easily to bust existing use.
Yes, as we saw when code disposal was introduced , even existing
applications can be ruined if care is not properly taken. Maybe if
create_tcl_sub() had been documented as part of the api this problem
would not have been created in the first place.
> 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().
Well I don't think CreateCommand() document vague: CLIENTDATA is
just any user private data to be pass to the callback function.
DELETEPROC is served as the destructor function that is to be called
when DeleteCommand() applied to the created tcl command. They are
optional it's no harm to set them undef if you do not know what it is.
> > > 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.
My idea is to apply a prefix to identify if first argument of call()
is a DESCRNAME. If the first arg call() is '%' , then it is a
DESCRNAME. (Let's forget previous AT sign '@' example, which might
confuse you). Otherwise the first arg is a tcl verb. For example the
following code:
call('%button1', ..., $sub1, sub{})
call('%button1', ..., $sub2, $sub3)
call('if', '1', $sub3)
:
code_dispose('button1');
Then inline sub{}, $sub1 and $sub2 would get disposed while #sub3 is
kept. The code looks much cleaner compared to annoying
TCLNAME/GENCODE/EVENT dealing and storing. Here one DESCRNAME maps
to multiple $subs, which is not allowed in current implementation.
Just share an idea for a better disposal interface.
And how would you implement this via Tkx?
for instance how do you modify a $menu>add_command(%args); call?
or $button =$in->new_ttk__button(-text=>$args{text}, -command=>$args{sub} );
like you said before if you dont know what to do with EVENT it can be
defined as undef, and then TCLNAME is the same as GENCODE (as the doc
says) and only that one item needs to be saved, just like you need
to save the CMDNAME passed to CreateCommand ()
Changing the existing and working interface to call() seems to be
much more error prone than documenting the interface to
create_tcl_sub(), just making things more complex.
As i already mentioned by using create_tcl_sub() to manage code
disposal, i can just add one call to create_tcl_sub() to lock down a
sub from code disposal, and add another call to _code_destroy() when
i want to release that coderef. Your proposed method requires every
call with a coderef to be modified which was one thing i was trying to avoid.
And you say "would get disposed", but when? the only time code may
get automagicly disposed is when something has the same DESCRNAME,
but here you propose allowing DESCRNAME ot have the same value over
multiple calls, are you proposing that inside the second call $sub1
and sub{}) get disposed? or are you proposing that $sub3 gets added
to that list for disposal later?
And a DESCRNAME can have more than one sub attached to it since
v1.06 What it is is that when the same DESCRNAME is seen again, the
prior code calls are now eligible for code destruction. One change i
made is to delay code destuction untill after the entire command is
parsed, so as not to have to destroy/recreate a coderef that already
exists in both the current and the prior call with the same DESCRNAME
This wasnt a problem at v1.02 because %anon_refs was only assigned to
if the TCLNAME did not exist already
> 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.
>
Yes, I think it is weird to return twin variable TCLNAME/GENCODE.
They are mostly the same thing and I need two variables to store
them. As a user, I'd like such function just return one thing/object
that can be pass to both call() and _code_dispose(). EVENT is also a
stuff that should not be worried by user: we are already familiar
with Tk::Ev() and it is also weird to provide yet another
variable/argument serving the same purpose.
It is not "ANOTHER variable/argument serving the same purpose" , it
is the already existing argument for that purpose and that is the
place it is specified to make it work like expected. As i pointed out
above, if you dont know what to do with EVENT just leave it as undef,
then TCONAME and GENCODE are the same and only one thing needs to be
retained. To removed EVENT from create_tcl_sub() would cause existing
code to need to be modified. I had considered creating a new
subroutine that did everything but EVENT/GENCODE and have
create_tcl_sub() use that for the meat of its work, but i was trying
to modify the existing structure as little as possible. Before the
mod to return both TCLNAME and GENCODE, when only GENCODE was
returnd, you had to manipulate gencode to get the TCLNAME if you
needed it. Since they really are two different things, TCLNAEM being
the %anan_refs location the definition is stored into, and GENCODE
is the call sequence inside tcl, i thought it made the most sense to
return both of them. I can forsee cases in the future where call()
would want the TCLNAME separate from GENCODE too. Part of the code
disposal problems introduced after v1.02 had to do with modifying
create_tcl_sub() to store its result in $anon_refs{DESCRNAME} rather
than $anon_refs{TCLNAME}. There were times i felt the best fix would
be to go back to %anon_refs only holding TCLNAMES and not DEscrNAMES,
and creating another place for code disposal to work from. In that
case you would want the TCLNAME separate from GENCODE so as to be
able to properly link the DESCRNAME storage back to the TCLNAME.
But i was not trying to change the way things were in v1.05, i was
trying to make them work again for existing v1.02 code. the change
had already been made to use DESCRNAME as the key for code disposal,
is i added back the concept of an entry for TCLNAMe as well. to keep
them separate i added '=' to the front of the DESCRNAMEs call() uses,
and specified that "::perl::' should not be used to start any
DESCRNAME. the developer may try to use.
Again let me point out that i did not introduce the code disposal
concept, i am just trying to make it work without busting existing
code. I have already pointed out that for the most part it is only
useful for cleaning up after "after" commands that use dynamically
scoped anon coderefs. Maybe what you need to do is specify what code
disposal should do, and how it is to be implemented. Otherwise i
think my patches have fixed the problem i encountered, have fixed
other problems if encountered later, and fit into the existing code
disposal model introduced after v1.02
As i have pointed out create_tcl_sub() is where code disposal it
implemented, and that being the case i feel that create_tcl_sub()
should be documented so as to let the developer control code
disposal. I think your proposed modifications to call() are
problematic and incur a greater risk of breaking something else.
> > > > 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.
>
>
> 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.
BTW, I found _code_dispose() has to be called in the form
'Tcl::_code_dispose(...)' rather than '$interp->_code_dispose(...)'.
It is documented, but it is counter-intuitive. Programmers can
hardly find their incorrect calling via $interp object. I suggest to
check input parameter and provide some warning at least. IMO,
%anon_refs{} would be better an object instance variable instead of
global one.
but it isnt an object instance now, as far as i can tell never has
been, So the change you are proposing here is not trivial. What
should it be an instance of?
While $interp can be considered an object all it is really is a
blessed scalar. So where do you propose you store this new object
instance? If we make $interp be the first key to %anon_refs that will
cause a lot of code to need to be changed.
And as i pointed out while delete_ref() is called with an $interp for
the most part that doesnt matter, since the $interp stored in the
Tcl::Code object is used for the deletion, and while there could be
many different interpreters, there is only one %anon_refs, so that on
a single TCLNAME can exist, there cannot be one for each interpreter.
Again, maybe the solution is to make a RFP asking for input on code
disposal, what it should do, how it should be called and what
modification will be needed to existing code. Without that my
intentions were to make it so existing v1.02 code still worked, and
there was a mechanism to control code disposal. I think my patches
have met both of those goals. (and as i watched code disposal in
action i realized it was destroying/recreating tcl subs over and over
and i wanted to prevent that too)
and i also realized that a deep bug in the xs code was leading to a
massive memory leak and fixed that as well.
SJ
