Ticket UUID: | 585105 | |||
Title: | CONST-ified parser | |||
Type: | Patch | Version: | CONSTify | |
Submitter: | dgp | Created on: | 2002-07-22 20:02:44 | |
Subsystem: | 45. Parsing and Eval | Assigned To: | dgp | |
Priority: | 9 Immediate | Severity: | ||
Status: | Closed | Last Modified: | 2002-08-05 10:27:35 | |
Resolution: | Accepted | Closed By: | dgp | |
Closed on: | 2002-08-05 03:27:35 | |||
Description: |
I've put together a patch that constifies the Tcl parser routines, and things that call them, like Tcl_Eval(). First here is a patch for the existing files... | |||
User Comments: |
dgp added on 2002-08-05 10:27:35:
Logged In: YES user_id=80530 committed for 8.4b2 release. hobbs added on 2002-08-05 05:23:36: Logged In: YES user_id=72656 let's just apply these and see what ripple effect it has. that will be the best way to test it, and it must be done pre-8.4b2 dgp added on 2002-08-03 05:17:37: File Deleted - 28298: File Added - 28301: tk-full.patch Logged In: YES user_id=80530 updated tk-full.patch includes changes to docs. dgp added on 2002-08-03 04:32:09: File Added - 28298: tk-full.patch dgp added on 2002-08-03 04:32:08: Logged In: YES user_id=80530 Finally, here is tk-full.patch that builds on the previous patch and fully CONST-ifies the Tk public interface as much as possible. dgp added on 2002-08-02 23:29:49: File Added - 28278: tk-alt.patch Logged In: YES user_id=80530 Here's an alternate patch for Tk. It fully migrates Tk to use the Tcl 8.4 interface (no USE_COMPAT_CONST). The 3 trouble spots where Tk scribbles on command arguments are resolved in this patch by simple copying to Tcl_DStrings. If that hurts performance, it should be resolvable with a further conversion to new Tcl_ObjTypes representing event sequences and text indices. dgp added on 2002-08-02 05:21:02: Logged In: YES user_id=80530 There are 3 potential problems with the Tk patch. The Tcl Patch CONST-ifies the Tcl_CmdProc typedef, so that the string arguments passed in as "argv" are not supposed to be scribbled on. The Tk patch here makes use of USE_COMPAT_CONST to trick the compiler into using the 8.3 definition of Tcl_CmdProc, so Tk will build fine. However, if another module is built using the 8.4 definition, and the compiler relies on the "const" nature of the arguments to store them in read-only memory, there could be a run time error if in fact Tk does attempt to scribble on any of the arguments passed to its commands. Tk in fact does make 3 such attempts: generic/tkBind.c, lines 4218, 4222: parsing an event specifier generic/tkTextIndex.c, lines 368,370,457,459,479,481,491,493: parsing text index strings generic/tkTest.c, line 2259: some strange conversion of \n <-> \0 A better patch for Tk will address these areas. msofer added on 2002-07-31 19:57:18: Logged In: YES user_id=148712 I have reviewed the tcl patch: A-OK under linux, which I guess covers all unices in this case. However, I did not test its functionality with extensions. Somebody else should look at: * the tk patch * Mac/Win compilation msofer added on 2002-07-31 05:22:56: File Added - 28094: complete.patch.gz jenglish added on 2002-07-30 08:48:09: File Added - 28006: tclVar.c.1.62.patch Logged In: YES user_id=68433 Oops, the patch is already out of date; there have been subsequent conflicting changes to tclVar.c. r1.61 fixed a bug that this patch also fixes, but in a different way. It (r1.61) also streamlined the final control flow, eliminating a 'goto'. r1.62 appears to be unrelated (at least there are no conflicts). Attached is a new patch to tclVar.c r1.62, incorporating the changes from 1.61 to 1.62 and the applicable changes from 1.60 to 1.61. jenglish added on 2002-07-30 07:30:57: Logged In: YES user_id=68433 Raising priority to make sure this gets considered for the 8.4b2 release (week of 5 Aug 2002). At least the parser and Tcl_Var* changes should be committed, even if the Tcl_CmdProc signature change is not. dgp added on 2002-07-25 08:44:25: File Deleted - 27624: File Added - 27709: complete.patch Logged In: YES user_id=80530 Here's an updated patch that fixes those problems. Passing on to maintainer for further review. I will be away until July 31. Work out further fixes amongs yourselves please. jenglish added on 2002-07-25 04:19:44: Logged In: YES user_id=68433 Reviewed 'complete.patch': this looks very good. The parser changes should definitely be committed; Tcl_Eval() and friends finally have the right signature, the patch fixes at least one known bug and probably several as-yet-undiscovered ones, and the code is now simpler and better-factored. With the patch, I was finally able to fully-CONSTify an extension that used to require -DUSE_NON_CONST. Just a few minor notes: In generic/tclInt.h, TclCmdProcType should be changed to match Tcl_CmdProc: typedef Tcl_CmdProc *TclCmdProcType; With the current definition (int (*TclCmdProcType)(...CONST char **argv)), there may be a type mismatch in extensions that #include tclInt.h with -DUSE_NON_CONST . Also, there's some stray debugging code (commented-out fprintfs) that made its way into the patch, and a comment in tclParseExpr.c/ParsePrimaryExpr() (~line 1390) that is no longer accurate. Other than that, I didn't see any problems. dgp added on 2002-07-24 10:52:57: File Added - 27625: tkcomplete.patch Logged In: YES user_id=80530 Here is a companion patch for Tk. dgp added on 2002-07-24 10:51:04: File Deleted - 27556: File Added - 27624: complete.patch dgp added on 2002-07-24 10:51:03: Logged In: YES user_id=80530 Here is an updated patch that completes CONST-ification of Tcl. It has a substantial parser re-write that operates on counted strings, and doesn't have the thread-safety issues of the last attempt. Also merged in are the features of Patch 582429 which CONST-ifies the Tcl_*Var* interfaces, and the patch attached to Bug 580433 that enhances the USE_NON_CONST macro to allows users of the C API to completely avoid all source incompatibilities with 8.3 headers if they need to. It's been said that CONST-ification needs to be an "all or nothing" affair. This patch lets us explore the "all" option. It's also been said that CONST-ification of the parser would kill performance. I don't see a significant problem with it. Simple testing with tclbench indicates the patched Tcl is overall faster than even the HEAD, and a still a significant improvement over 8.4b1 (thanks, MIguel!). There are some things that are slower, notably evaluation of unbraced [expr]'s, but haven't we advised against those for awhile now? Anyhow the main point was to get something working so that we can decide whether or not to adopt it based on measurements rather than speculation, so please measure away! Try it. dgp added on 2002-07-23 03:04:43: File Added - 27556: tclParse.h Logged In: YES user_id=80530 ...and here is a new file tclParse.h to place in the generic directory. Apply and `make genstubs', then try it. Note that the patched code will not be thread-safe. Further revisions required for that. dgp added on 2002-07-23 03:02:45: File Added - 27555: parser.patch |
Attachments:
- tk-full.patch [download] added by dgp on 2002-08-03 05:17:37. [details]
- tk-alt.patch [download] added by dgp on 2002-08-02 23:29:49. [details]
- complete.patch.gz [download] added by msofer on 2002-07-31 05:22:56. [details]
- tclVar.c.1.62.patch [download] added by jenglish on 2002-07-30 08:48:09. [details]
- complete.patch [download] added by dgp on 2002-07-25 08:44:25. [details]
- tkcomplete.patch [download] added by dgp on 2002-07-24 10:52:57. [details]