Ticket UUID: | e711ffb4583d8183a57cba36ab197c18721f6049 | |||
Title: | TclIsLocalScalar() appears wrongheaded | |||
Type: | Bug | Version: | all | |
Submitter: | dgp | Created on: | 2014-11-26 20:08:15 | |
Subsystem: | - New Builtin Commands | Assigned To: | dgp | |
Priority: | 5 Medium | Severity: | Minor | |
Status: | Closed | Last Modified: | 2014-12-23 02:42:55 | |
Resolution: | Fixed | Closed By: | dgp | |
Closed on: | 2014-12-23 02:42:55 | |||
Description: |
While looking at [d2ffcca163] there was briefly a false lead that the trouble was caused by TclIsLocalScalar(). That was wrong, but prompted a look at what TILS() was doing since it appeared to be doing nothing that made much sense. All callers of TILS are in tclCompCmds.c -- routines that compile commands into bytecode. All those routines operate on Tcl_Token streams as input. The parser has already had its turn. Every TILS call is on strings that are already marked by the parser as being SIMPLE_WORD. If they have special characters, we already know at this point they've been quoted so as to neuter their specialness. What's wanted where TILS calls are made is a simpler task of avoiding arrays and qualified variable names, and the right tool for that appears to be PushVarName (still confirming that). Here's a demo of what's (harmlessly) wrong. First a base case for comparison: % proc demo {} {catch {} {r}; puts '${r}'} % tcl::unsupported::disassemble proc demo ByteCode 0x0x1779d4b0, refCt 1, epoch 3, interp 0x0x17748250 (epoch 3) Source "catch {} {r}; puts '${r}'" Cmds 2, src 25, inst 36, litObjs 4, aux 0, stkDepth 4, code/src 0.00 Proc 0x0x17774b40, refCt 1, args 0, compiled locals 1 slot 0, scalar, "r" Exception ranges 1, depth 1: 0: level 0, catch, pc 5-12, catch 11 Commands 2: 1: pc 0-22, src 0-11 2: pc 23-34, src 14-24 Command 1: "catch {} {r}" (0) beginCatch4 0 (5) push1 0 # "" (7) push1 1 # "0" (9) jump1 +4 # pc 13 (11) pushResult (12) pushReturnCode (13) endCatch (14) reverse 2 (19) storeScalar1 %v0 # var "r" (21) pop (22) pop Command 2: "puts '${r}'" (23) push1 2 # "puts" (25) push1 3 # "'" (27) loadScalar1 %v0 # var "r" (29) push1 3 # "'" (31) concat1 3 (33) invokeStk1 2 (35) done And now nearly the same case with TILS() taking us down the wrong path: % proc demo {} {catch {} {[}; puts '${[}'} % tcl::unsupported::disassemble proc demo ByteCode 0x0x1779cbe0, refCt 1, epoch 3, interp 0x0x17748250 (epoch 3) Source "catch {} {[}; puts '${[}'" Cmds 2, src 25, inst 22, litObjs 5, aux 0, stkDepth 4, code/src 0.00 Proc 0x0x17772930, refCt 1, args 0, compiled locals 1 slot 0, scalar, "[" Commands 2: 1: pc 0-8, src 0-11 2: pc 9-20, src 14-24 Command 1: "catch {} {[}" (0) push1 0 # "catch" (2) push1 1 # "" (4) push1 2 # "[" (6) invokeStk1 3 (8) pop Command 2: "puts '${[}'" (9) push1 3 # "puts" (11) push1 4 # "'" (13) loadScalar1 %v0 # var "[" (15) push1 4 # "'" (17) concat1 3 (19) invokeStk1 2 (21) done Whether the variable name is "r" or "[" makes no difference, so we're falling back to command invocation for no good reason. TILS is testing the wrong thing for the wrong reason and generating less good (though correct) code as a result. | |||
User Comments: |
dgp added on 2014-12-23 02:42:55:
and now on trunk too. dgp added on 2014-12-19 16:09:28: Corrected in 8.5 branch dkf added on 2014-12-12 02:51:52: (text/html) Don't forget that the trunk may have additional uses, as the set of compiled things expanded after 8.5. Much of my use of such things as TILS in command compilers is by using existing functioning patterns, rather than analysing what things do in great depth. |