tcl-core

[TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Jan N. <jan...@gm...>]

2008/11/23 Joe English <jen...@fl...>:
>
> Jan Nijtmans wrote:
>> This is a Call For Votes on TIP #340.
>>
>>  TIP #340: CONST Qualification of Tcl_SetResult's argument and -Wwrite-string
>
> TIP#340: NO.
>
> I support the goals of this TIP, but not the specific
> proposed refactoring.  It leaves Tcl_SetResult() in a
> broken state: with an incorrect const qualifier on the
> second argument and a worthless vestigial third argument.

Understandably (is that good english?...) there were no
other votes on this, and after Joe's response Joe and I
have been working to resolve our 'conflict'. Therefore I
decided to modify the TIP along the following lines.

- The Tcl_SetResult signature will not change, but this
  function will be deprecated in full. It will be replaced
  by a macro:
      #define Tcl_SetStringResult(interp, s) \
      Tcl_SetObjResult(interp, Tcl_NewStringObj(s, -1))

This allows to modify almost all Tcl_SetResult() calls
in the core to Tcl_SetStringResult(), except for the
ones in tclResult.c and tclTest.c. All tests pass,
and all extensions compiled against Tcl 8.5 still
run unmodified with Tcl 8.6. It will still be a lot of
work to check all Tcl_SetResult() calls, because
a simple change to Tcl_SetStringResult() is not
always the best solution. (I hope that Joe will
help me with this.....)

The disadvantage is that it might break extensions
compiled against Tcl 8.6 which still use interp->result,
but since TIP #330 is accepted, who cares!

I will create a new patch this weekend, so the vote
can go on. But then, monday is a little short.
So, I would like to delay the vote until:

    Monday dec 1st, 12:00 GMT (i.e. [clock format 1228478400]).

Regards,
       Jan Nijtmans

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Kevin K. <ke...@ac...>]

Jan Nijtmans wrote:
> Understandably (is that good english?...) there were no
> other votes on this, and after Joe's response Joe and I
> have been working to resolve our 'conflict'. Therefore I
> decided to modify the TIP along the following lines.
> 
> - The Tcl_SetResult signature will not change, but this
>   function will be deprecated in full. It will be replaced
>   by a macro:
>       #define Tcl_SetStringResult(interp, s) \
>       Tcl_SetObjResult(interp, Tcl_NewStringObj(s, -1))

This is nice.

I don't think there's any controversy that
we want to kill Tcl_SetResult -- eventually. It isn't const-
correct, and can't be made const-correct. But, aside from
spewing compiler warnings, it's Mostly Harmless. The chief
reason for wanting to kill it is that it's all tied up in
the manipulations of interp->result, and we have to wait at
least another release cycle before we can put paid to them.

Unfortunately, there is a lot of Tcl_SetResult out there,
chiefly because it was a good deal less typing than
Tcl_SetObjResult(interp, Tcl_NewStringObj(..., -1).
Tcl_SetStringResult would be a cleaner alternative.

--
73 de ke9tv/2, Kevin

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Joe E. <jen...@fl...>]

Jan Nijtmans wrote:
>
> [ Revised specification for TIP#340, of which I approve ]

Just a few notes:

> It will still be a lot of
> work to check all Tcl_SetResult() calls, because
> a simple change to Tcl_SetStringResult() is not
> always the best solution. (I hope that Joe will
> help me with this.....)

To clarify: this is not a mechanical change, but
the whole point of enabling additional compiler warnings --
to improve the code -- involves a certain degree of code
review in the first place.

Tcl_SetResult(interp, s, TCL_STATIC|TCL_VOLATILE);
can generally be shortened to Tcl_SetStringResult(interp, s);
without further thought.

In the case of Tcl_SetResult(interp, (char*)s, TCL_STATIC|TCL_VOLATILE);
where "s" is a const char *, you can also remove the cast-away-const cast.

Calls that use TCL_DYNAMIC or a general freeProc need to be
examined on a case-by-case basis.

Also: this is not a forced change; it requires no changes
to extensions or (strictly speaking) even to the core itself.
The idea is to provide a convenient, const-correct alternative
to Tcl_SetResult() for the benefit of code that wishes to become
const- and -Wwrite-strings safe.


> The disadvantage is that it might break extensions
> compiled against Tcl 8.6 which still use interp->result,
> but since TIP #330 is accepted, who cares!

To clarify: such extensions are already broken and
have been in danger of the brokenness being exposed
at any time anyway.  Extensions that look directly
at interp->result may be affected simply because there
will be even fewer places in the core that set interp->result;
this sort of thing has been going on since Tcl 8.0,
so there's nothing new here.


--Joe English

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Jan N. <nij...@us...>]

2008/11/27 Jan Nijtmans <jan...@gm...>:
> I will create a new patch this weekend, so the vote
> can go on. But then, monday is a little short.
> So, I would like to delay the vote until:
>
>    Monday dec 1st, 12:00 GMT (i.e. [clock format 1228478400]).

This clock format does not match the specified date. Of
course I meant:

    Friday dec 5th, 12:00 GMT (i.e. [clock format 1228478400]).

Regards,
        Jan Nijtmans

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Pat T. <pat...@us...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jan Nijtmans wrote:
> 2008/11/27 Jan Nijtmans <jan...@gm...>:
>> I will create a new patch this weekend, so the vote
>> can go on. But then, monday is a little short.
>> So, I would like to delay the vote until:
>>
>>    Monday dec 1st, 12:00 GMT (i.e. [clock format 1228478400]).
> 
> This clock format does not match the specified date. Of
> course I meant:
> 
>     Friday dec 5th, 12:00 GMT (i.e. [clock format 1228478400]).
> 
> Regards,
>         Jan Nijtmans

This kind of error seems to crop up regularly. How about everyone stops
being clever and just writes in the date so that people can just read
it. Use of seconds to describe a date in messages designed to be read by
humans is dumb.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQCVAwUBSS8vMWB90JXwhOSJAQIVDQP/TMsMDZ6tEwCyIB2UiG+Mh4bOQfChgi9E
b2y4JXwv1k/usDgNLPObP+vegs6osLdnpIzWdFUyx1hzdJLVRhuiYUBdSZ9S1KeZ
F5+nxFBiNBzStaWe+VztPi113ivht6MUUYc6TEKWCuG0tRO9wAu4/mLM7Oh2cIpk
E1KF6wSa6+4=
=xgJQ
-----END PGP SIGNATURE-----

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Donal K. F. <don...@ma...>]

Pat Thoyts wrote:
> This kind of error seems to crop up regularly. How about everyone stops
> being clever and just writes in the date so that people can just read
> it. Use of seconds to describe a date in messages designed to be read by
> humans is dumb.

FWIW, I (almost) always cut-n-paste a [clock format] that's giving the
time I want when I test it out.

Donal.

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Kevin K. <ke...@ac...>]

Jan Nijtmans:
>> This clock format does not match the specified date. Of
>> course I meant:
>>
>>     Friday dec 5th, 12:00 GMT (i.e. [clock format 1228478400]).

Pat Thoyts:
> This kind of error seems to crop up regularly. How about everyone stops
> being clever and just writes in the date so that people can just read
> it. Use of seconds to describe a date in messages designed to be read by
> humans is dumb.

Note that in this particular case, it was the human-readable form
that was incorrect. I'm not sure what conclusion is to be drawn.

-- 
73 de ke9tv/2, Kevin

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Jan N. <nij...@us...>]

2008/11/27 Joe English <jen...@fl...>:
>
> Jan Nijtmans wrote:
>>
>> [ Revised specification for TIP#340, of which I approve ]
>

First remark: This looks like a YES, but still
isn't. I'm wondering why still no other votes then
mine came out. Was it because the TIP was still
not updated to include the results of the agreement?
If that's the case, I have good news: the TIP
text is updated now!

Second remark: I agree with all of Joe's remarks
below. Especially the last remark is important.
Although the risk is not zero, it is small enough
in my opinion to justify a YES. My "who cares!"
remark was intended to trigger everyone's
attention, not to make people afraid of voting. :-)
Joe was the only one who reacted, and he
reacted exactly the way I hoped for.....

> Just a few notes:
>
>> It will still be a lot of
>> work to check all Tcl_SetResult() calls, because
>> a simple change to Tcl_SetStringResult() is not
>> always the best solution. (I hope that Joe will
>> help me with this.....)
>
> To clarify: this is not a mechanical change, but
> the whole point of enabling additional compiler warnings --
> to improve the code -- involves a certain degree of code
> review in the first place.
>
> Tcl_SetResult(interp, s, TCL_STATIC|TCL_VOLATILE);
> can generally be shortened to Tcl_SetStringResult(interp, s);
> without further thought.
>
> In the case of Tcl_SetResult(interp, (char*)s, TCL_STATIC|TCL_VOLATILE);
> where "s" is a const char *, you can also remove the cast-away-const cast.
>
> Calls that use TCL_DYNAMIC or a general freeProc need to be
> examined on a case-by-case basis.
>
> Also: this is not a forced change; it requires no changes
> to extensions or (strictly speaking) even to the core itself.
> The idea is to provide a convenient, const-correct alternative
> to Tcl_SetResult() for the benefit of code that wishes to become
> const- and -Wwrite-strings safe.
>
>
>> The disadvantage is that it might break extensions
>> compiled against Tcl 8.6 which still use interp->result,
>> but since TIP #330 is accepted, who cares!
>
> To clarify: such extensions are already broken and
> have been in danger of the brokenness being exposed
> at any time anyway.  Extensions that look directly
> at interp->result may be affected simply because there
> will be even fewer places in the core that set interp->result;
> this sort of thing has been going on since Tcl 8.0,
> so there's nothing new here.

Regards,
         Jan Nijtmans

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Jan N. <nij...@us...>]

Here is the results of TIP #340 vote (Provided I
interpreterd JE's correctly.

JN: YES
JE: NO

This means the TIP is rejected.
The best guess about the why question is Kevin's remark:

Kevin Kenny wrote:
> I don't think there's any controversy that
> we want to kill Tcl_SetResult -- eventually. It isn't const-
> correct, and can't be made const-correct. But, aside from
> spewing compiler warnings, it's Mostly Harmless. The chief
> reason for wanting to kill it is that it's all tied up in
> the manipulations of interp->result, and we have to wait at
> least another release cycle before we can put paid to them.

The original TIP specification was too simple. The revised one
too complex. I didn't hear any arguments that this TIP was
fundamentally incorrect, so I propose to put back this TIP
and re-examine it again for Tcl 8.7.

Regards,
         Jan Nijtmans

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Joe E. <jen...@fl...>]

Jan Nijtmans wrote:

> Here is the results of TIP #340 vote (Provided I
> interpreterd JE's correctly.
>
> JN: YES
> JE: NO
>
> This means the TIP is rejected.
>
> The best guess about the why question is Kevin's remark:

Erm...

For my part, this is the main reason:

| jenglish@eurydice$ folder
| inbox+ has 283 messages  (1-425); cur=377.

I saw the note mentioning that the TIP had been updated,
took a quick look, saw an ABSTRACT section and and a
RATIONALE section but no SPECIFICATION; thought "OK,
it'll take some effort to figure out *exactly* what's
being voted on, will get to it later", and have not
gotten around to doing so yet.

Sorry about that.  It's just that:

| jenglish@eurydice$ folder
| inbox+ has 283 messages  (1-425); cur=377.

For what it's worth, I would vote YES on the following:


| SPECIFICATION
|
| Add a new convenience routine to the public API:
|
|	void Tcl_SetStringResult(Tcl_Interp *interp, const char *result);
|
| which shall be equivalent to
|
|	Tcl_SetObjResult(interp, Tcl_NewStringObj(result, -1));
|
| The existing routine Tcl_SetResult() shall be formally deprecated.
|
| END SPECIFICATION.


> Kevin Kenny wrote:
> > I don't think there's any controversy that
> > we want to kill Tcl_SetResult -- eventually. It isn't const-
> > correct, and can't be made const-correct. But, aside from
> > spewing compiler warnings, it's Mostly Harmless. The chief
> > reason for wanting to kill it is that it's all tied up in
> > the manipulations of interp->result, and we have to wait at
> > least another release cycle before we can put paid to them.

I don't think that's exactly right -- Jan and I have identified
a path towards eliminating interp->result internal manipulations,
and I don't think it needs to wait a complete release cycle (all
of the changes are internal and could be done at any time).
Tcl_SetStringResult() would help smooth the path is all.



--Joe English

  jen...@fl...

Re: [TCLCORE] Delaying CVF#340 until dec 5 (Re: CFV TIP #340)

[Jan N. <nij...@us...>]

2008/12/8 Joe English <jen...@fl...>:
> For what it's worth, I would vote YES on the following:
>
>
> | SPECIFICATION
> |
> | Add a new convenience routine to the public API:
> |
> |       void Tcl_SetStringResult(Tcl_Interp *interp, const char *result);
> |
> | which shall be equivalent to
> |
> |       Tcl_SetObjResult(interp, Tcl_NewStringObj(result, -1));
> |
> | The existing routine Tcl_SetResult() shall be formally deprecated.
> |
> | END SPECIFICATION.

Yes, that's it, only that I specified Tcl_SetStringResult() to be
a macro, doing exactly that. The reason is that I expect
Tcl_SetStringResult to be a popular function, and someone
accidently using it might find that extensions using it,
compiled against Tcl 8.6, will no longer run against Tcl 8.5.
I know that 'forward compatibility' is not promised, but for
a function that is expected to be 'popular' it is more likely
to become a problem that for a hardly used function.

> I don't think that's exactly right -- Jan and I have identified
> a path towards eliminating interp->result internal manipulations,
> and I don't think it needs to wait a complete release cycle (all
> of the changes are internal and could be done at any time).
> Tcl_SetStringResult() would help smooth the path is all.

Agreed. But it would have helped if other TCT members
joined the discussion.

The exact specification can be found as 340b.patch in
[Patch 2315890].

So, what to do now? How do others feel about it? Should
I delay the vote again? Is Tcl_SetStringResult as a macro
OK, or should we make it a function (accepting the
'forward incompatibility'). Or both: define the function, but
let it be a macro in Tcl 8.6.

I called the vote in order to be able to reach the 9 dec deadline.
Would that still be possible now?

Regards,
         Jan Nijtmans