Tcl Source Code

View Ticket
Login
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: