Help with tcl internals for proposed TIP

[ted brown]

A tcl error for extra characters following a } was mentioned. I believe
I’ve been able to make a change to improve the error message in
tclParse.c as shown below.

However, I am unsure if it should make a call to decr a reference count,
as I’ve seen mentioned in other similar code. I need someone with more
experience with this to verify I did this correctly before making the
proposal.

In the below, I have only so far changed it for the error with a close
brace, which was previously the same as the code for the close quote
(except the slight difference in text and the constant).

if (src[-1] == '"') {
if (interp != NULL) {
Tcl_SetObjResult(interp, Tcl_NewStringObj(
"EXtra characters after close-quote", -1));
}
parsePtr->errorType = TCL_PARSE_QUOTE_EXTRA;
} else {
if (interp != NULL) {
Tcl_Obj *emessage = NULL;
emessage = Tcl_NewStringObj(
"eXtra characters after close-brace at }", -1);
Tcl_AppendLimitedToObj(emessage,src,-1,10,NULL);

Tcl_SetObjResult(interp,emessage);
// Tcl_DecrRefCount(emessage); is this needed?
}
parsePtr->errorType = TCL_PARSE_BRACE_EXTRA;
}

-------------------------------------------------------------

[1817]$ rlwrap -pred ./tclsh
% foo {a b c}def g h i
eXtra characters after close-brace at }def g h i
% foo {a b c}d
eXtra characters after close-brace at }d
% foo {a b c}defghijklmnop q r s t
eXtra characters after close-brace at }defghij...
% foo {a b c}
invalid command name "foo"
% foo "a b c"d e f g
EXtra characters after close-quote
% proc bar {} {foo {a b c}d}
% bar
eXtra characters after close-brace at }d


(ignore the upper case E and X used for testing)

[Rich]

ted brown <tedbr...@gmail.com> wrote:
> A tcl error for extra characters following a } was mentioned. I believe

> I?ve been able to make a change to improve the error message in

> tclParse.c as shown below.
>
> However, I am unsure if it should make a call to decr a reference count,

> as I?ve seen mentioned in other similar code. I need someone with more

> experience with this to verify I did this correctly before making the
> proposal.
>
> In the below, I have only so far changed it for the error with a close
> brace, which was previously the same as the code for the close quote
> (except the slight difference in text and the constant).
>
> if (src[-1] == '"') {
> if (interp != NULL) {
> Tcl_SetObjResult(interp, Tcl_NewStringObj(
> "EXtra characters after close-quote", -1));
> }
> parsePtr->errorType = TCL_PARSE_QUOTE_EXTRA;
> } else {
> if (interp != NULL) {
> Tcl_Obj *emessage = NULL;
> emessage = Tcl_NewStringObj(
> "eXtra characters after close-brace at }", -1);
> Tcl_AppendLimitedToObj(emessage,src,-1,10,NULL);
>
> Tcl_SetObjResult(interp,emessage);
> // Tcl_DecrRefCount(emessage); is this needed?
> }
> parsePtr->errorType = TCL_PARSE_BRACE_EXTRA;
> }

man Tcl_NewStringObj:

Tcl_NewStringObj and Tcl_NewUnicodeObj return a pointer to a newly
created value with reference count zero.

man Tcl_SetObjResult

Tcl_SetObjResult arranges for objPtr to be the result for
interp, replacing any existing result. The result is left pointing
to the value referenced by objPtr. objPtr's reference count is
incremented since there is now a new reference to it from interp.

So, you started at zero, then you gave a 'copy' to the interp, ref
count now one. The 'obj' is now owned by the interp, I believe you
should not need the "is this needed" line.

[ted brown]

Thanks Rich, I confess I don't have a good understanding of the obj or
reference count system in Tcl. In searching the code, I found this
comment block, and the Notes section is what I was going on:


/*
*----------------------------------------------------------------------
*
* TclLindexFlat --
*
* This procedure is the core of the 'lindex' command, with all index
* arguments presented as a flat list.
*
* Results:
* Returns a pointer to the object extracted, or NULL if an error
* occurred. The returned object already includes one reference count for
* the pointer returned.
*
* Side effects:
* None.
*
* Notes:
* The reference count of the returned object includes one reference
* corresponding to the pointer returned. Thus, the calling code will
* usually do something like:
* Tcl_SetObjResult(interp, result);
* Tcl_DecrRefCount(result);
*
*----------------------------------------------------------------------
*/



But if you think this is a different situation here, I think I could
just submit what I have as a TIP proposal, and I'm sure one of the core
team would know right off if I have it correct. But I think I'll wait a
few days to see if someone else chimes in.

I was also a bit unsure that the pointer src would be guaranteed to be a
pointer to a text string ending with a null so that even with the limit
of 10 bytes in the call to Tcl_AppendLimitedToObj, it won't get an
access violation on a short command.

[Donal K. Fellows]

On Saturday, 3 April 2021 at 20:32:36 UTC+1, tedbr...@gmail.com wrote:
> In the below, I have only so far changed it for the error with a close
> brace, which was previously the same as the code for the close quote
> (except the slight difference in text and the constant).
>
> if (src[-1] == '"') {
> if (interp != NULL) {
> Tcl_SetObjResult(interp, Tcl_NewStringObj(
> "EXtra characters after close-quote", -1));
> }
> parsePtr->errorType = TCL_PARSE_QUOTE_EXTRA;
> } else {
> if (interp != NULL) {
> Tcl_Obj *emessage = NULL;
> emessage = Tcl_NewStringObj(
> "eXtra characters after close-brace at }", -1);
> Tcl_AppendLimitedToObj(emessage,src,-1,10,NULL);
>
> Tcl_SetObjResult(interp,emessage);
> // Tcl_DecrRefCount(emessage); is this needed?
> }
> parsePtr->errorType = TCL_PARSE_BRACE_EXTRA;
> }

You don't need to explicitly manage the reference count of emessage; you're handing it pretty much directly to the interpreter and it does all the reference count management you need. (This sort of consideration is why Tcl's references start at zero and not one.)

Your code (omitting the rest for brevity) could be just:

if (interp != NULL) {
Tcl_Obj *emessage = Tcl_NewStringObj(
"extra characters after close-brace at }", -1);

Tcl_AppendLimitedToObj(emessage, src, -1, 10, NULL);
Tcl_SetObjResult(interp, emessage);
}

It does NOT need a TIP; the exact format of error messages is not subject to those rules. It DOES need testing to make sure it picks up the correct bit of text. (I don't think it needs much testing as it is an “obviously correct” sort of change and existing tests might cover it.)

Donal.

[ted brown]

Ah, thanks. I had sent it off to the core mailing list before I saw your
reply. I keep forgetting about the time zone difference. I sent it in
with a TIP formatted section, in case that was needed.

So, the final version for both quotes and braces could then be (bumping
the # of chars to show before using the ... to say 20 since 10 is likely
too small) and with some extra spacing as you used,

if (src[-1] == '"') {
if (interp != NULL) {

Tcl_Obj *emessage = Tcl_NewStringObj(
"extra characters after close-quote at \"", -1);
Tcl_AppendLimitedToObj(emessage, src, -1, 20, NULL);
Tcl_SetObjResult(interp, emessage);

}
parsePtr->errorType = TCL_PARSE_QUOTE_EXTRA;
} else {
if (interp != NULL) {

Tcl_Obj *emessage = Tcl_NewStringObj(
"extra characters after close-brace at }", -1);

Tcl_AppendLimitedToObj(emessage, src, -1, 20, NULL);
Tcl_SetObjResult(interp, emessage);
}
parsePtr->errorType = TCL_PARSE_BRACE_EXTRA;
}


Since it is using src[-1] to test against the " I surmised that src[0]
would be the first character following the close brace or close quote in
the text being parsed, and when testing it produced the expected result.
Perhaps someone more familiar with the code there would know for certain.

If a TIP is not needed, then was that the right approach, to email it to
the core's email list?

[ted brown]

On 4/6/2021 8:53 AM, ted brown wrote:

>
> So, the final version for both quotes and braces could then be (bumping
> the # of chars to show before using the ... to say 20 since 10 is likely
> too small) and with some extra spacing as you used,

>             Tcl_AppendLimitedToObj(emessage, src, -1, 20, NULL);

I just tested with the code that was the error in the post by Camilo
that was the incentive for this change, and now see this, but by using
30 instead.

The ... is counted as part of the 30, so I am leaning more towards this
larger number for max chars here. The other parts of the message seem to
be using an even larger value, but 30 seems about right for this part of it.

[1819]$ rlwrap -pred ./tclsh testem.tcl
extra characters after close-brace at }else {
for {set j 1}...
while executing
"if {$i==1} {
for {set j 1} {$j <= $numEleY} {incr j 1} {

set nI [expr 10*$j - 9]
set nJ [expr $nI + 2]
set ..."
("for" body line 3)
invoked from within
"for {set i 1} {$i <= 2} {incr i 1} {

if {$i==1} {
for {set j 1} {$j <= $numEleY} {incr j 1} {

set nI [expr 10*$j - 9]
..."
(file "testem.tcl" line 1)