Ticket UUID: | 1290457 | |||
Title: | TEOVI refactoring | |||
Type: | Patch | Version: | None | |
Submitter: | tallniel | Created on: | 2005-09-13 21:06:36 | |
Subsystem: | 45. Parsing and Eval | Assigned To: | tallniel | |
Priority: | 5 Medium | Severity: | ||
Status: | Closed | Last Modified: | 2009-03-03 00:28:58 | |
Resolution: | Out of Date | Closed By: | tallniel | |
Closed on: | 2009-03-02 17:28:58 | |||
Description: |
This patch refactors TclEvalObjvInternal into a bunch of distinct functions to handle command lookup, enter traces, exit traces, unknown commands, and actual invocation. I think the resultant code is clearer and easier to maintain. Passes the test-suite, and should behave identically to the previous code. | |||
User Comments: |
tallniel added on 2009-03-03 00:28:58:
allow_comments - 1 I believe the NRE changes make this patch obsolete now. tallniel added on 2006-07-08 21:42:37: File Added - 184266: teovi-08July06.patch Logged In: YES user_id=102050 I've added a new patch (teovi-08July06.patch) which converts TclObjInvoke, InvokeImportedCmd and TclInvokeObjectCommand (needed?) to use the new TclInvokeCommand. This passes the test-suite and passes the original code for bug 582506. I put the declaration for TclInvokeCommand in tclInt.h for now. dgp added on 2006-07-07 00:12:42: Logged In: YES user_id=80530 What's the right way for TclObjInvoke() to use the new routines instead of calling cmdPtr->objProc directly? Can an updated patch make that change? Likewise for InvokeImportedCmd() in tclNamespace.c tallniel added on 2006-06-30 17:47:31: File Added - 183405: teovi-29Jun06.patch Logged In: YES user_id=102050 Uploading newer patch: The new patch refactors things again to create a new TclInvokeCommand function that can be used in place of the various places where the core directly calls a cmdPtr->objProc (e.g. 582506). The new function is not declared "static", but I've not put a declaration in any header file as I wasn't sure which would be appropriate (tclCompile.h?). For instance, to fix 582506 you can change InvokeImportedCmd (in tclNamesp.c) from: return (*realCmdPtr->objProc)(realCmdPtr->objClientData, interp, objc, objv); to: return TclInvokeCommand(interp, objc, objv, realCmdPtr, 0); (Actually, this only fixes the original problem of 582506 -- miguel's added comment is not fixed as that requires further changes to how execution traces are stored/invoked). I've also added a TODO comment in the source for TclEvalObjvInternalUnknownHandler. In the original code this calls Tcl_GetCommandFromObj to check if the handler exists and then calls TEOVI to invoke it (which then calls T_GCFO again). I think this can be replaced by a call to the new TclInvokeCommand, which would avoid the extra call to T_GCFO as we could just pass in the Command from the previous call. However, the call to TEOVI also passes in the *original* command+args string when calling the unknown handler. I'm not sure if this is ever actually used (it is passed to further execution trace handlers, but I haven't dug further), so I'm not sure if this replacement can be made safely. The new patch passes the test-suite, *EXCEPT* it causes a segfault in one test in stack.test on my system, but only when symbols are *disabled*. The segfault is from blowing the C stack, and is obviously caused by the extra function call added to the stack (TEOVI -> TEOVIInvokeCommand). Adding an "inline" keyword to TEOVIIC fixes this for me, but is C99/GCC-specific. Alternatives would be either to inline this function manually again (still leaves some refactoring, which may be worth it), or a further refactoring. The further refactoring would split into before/after functions for taking care of traces etc, but leave the actual calling of cmdPtr->objProc to the caller (TEOVI or TclInvokeCommand). I can produce a further patch for this if you want. dgp added on 2006-06-14 23:09:59: Logged In: YES user_id=80530 This patch may contain the seeds for a fix for 582506. tallniel added on 2006-06-14 02:37:08: File Added - 181538: teovi-13Jun06.patch tallniel added on 2006-06-14 02:37:07: Logged In: YES user_id=102050 Updated patch for latest CVS HEAD (*not* dgp-refactor). tallniel added on 2005-09-14 04:40:00: File Added - 148982: teovi-dgp-refactor.patch tallniel added on 2005-09-14 04:06:38: File Added - 148976: teovi.patch |
Attachments:
- teovi-08July06.patch [download] added by tallniel on 2006-07-08 21:42:37. [details]
- teovi-29Jun06.patch [download] added by tallniel on 2006-06-30 17:47:31. [details]
- teovi-13Jun06.patch [download] added by tallniel on 2006-06-14 02:37:08. [details]
- teovi-dgp-refactor.patch [download] added by tallniel on 2005-09-14 04:39:50. [details]
- teovi.patch [download] added by tallniel on 2005-09-14 04:06:37. [details]