tcl-core

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

[Andreas L. <av...@lo...>]

"Jan Nijtmans" <jan...@gm...> wrote:
> 2008/11/17 Joe English <jen...@fl...>:
> > Specifically: with the proposed change,
> >    Tcl_SetResult(interp, "Hi!", TCL_DYNAMIC);
> > would not raise a compiler error even with -Wwrite-strings.
> > That's a step backwards.

I don't know, if Tcl_SetResult  has still any value apart from
compatibility.  If it has, then it might be worth a consideration
adding a variant like "Tcl_SetResultRO"(bikeshed-warning) that will
take a const char*, and a pointer to a function that in turn will
also take a const-pointer. That means, there'd also be a 
typedef void (Tcl_FreeProcRO) _ANSI_ARGS_((const char *blockPtr));
and TCL_STATIC would be eventually be typed as Tcl_FreeProcRO, so
it could be passed to the const char* taking Tcl_SetResultRO().

This appears to me to avoid the "lying to the compiler", by splitting
up the Tcl_SetResult into one for the const-cases and one for general
cases.   Anyone who cares about the compiler warnings (hopefully
including the core) could then use the appropriate one, if creating
a TclObject and using the "Obj" variant is not desired..

It may of course be, that if one changes a source at all, he should
really change it to the "Obj" variant and also make sure that he doesn't
use interp->result, and also clean up all further legacy-stuff here and
there in the course... That would of course void my suggestion, but
possibly be too much work for old code to be reasonable.

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

[Donald G P. <dg...@ni...>]

Jan Nijtmans wrote:
 > In a quick scan, I can only find 4 Tcl_SetResult calls in the
 > core in which the 3th argument is not 0 or TCL_VOLATILE:
 >    tclDictObj.c (1 line)
 >    tclTest.c (2 lines)
 >    tclThreadTest.c (1 line)

There's another in tkOldConfig.c.  And one in tkMacOSXWm.c.
And tkUnixWm.c.  And tkWinWm.c

-- 
| Don Porter          Mathematical and Computational Sciences Division |
| don...@ni...             Information Technology Laboratory |
| http://math.nist.gov/~DPorter/                                  NIST |
|______________________________________________________________________|

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

[Andreas L. <av...@lo...>]

Joe English <jen...@fl...> noticed:
> Andreas Leitgeb wrote:
> > typedef void (Tcl_FreeProcRO) _ANSI_ARGS_((const char *blockPtr));
> > and TCL_STATIC would be eventually be typed as Tcl_FreeProcRO, so
> > it could be passed to the const char* taking Tcl_SetResultRO().

> A read-only free procedure "void (*Tcl_FreeProcRO)(const char *blockPtr);"
> is oxymoronic -- "const" means "I will not modify this argument"
> and freeing memory counts as modification.

It surely would be oxymoronic for a real free()-ing function, but isn't
so for mere dummy values dressed up like pointers to such a function,
or for special custom handlers that just want to be notified when the
object *would* be freed.

> Simpler: Tcl_SetResultRO wouldn't need or take a third 'freeProc'
> argument, ...

My point was, that the Tcl_SetResultRO would only take those freeProcs
that declared themselves as not ever harming the target of the pointer.
Whether that is limited to TCL_STATIC and TCL_VOLATILE (as far as I've
gathered from this thread), or also may include special custom handlers
is currently beyond me, but even if it's just those two, then these two
cases still have to be separated for Tcl_SetResultRO().

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

2008/11/17 Donald G Porter <dg...@ni...>:
> There's another in tkOldConfig.c.  And one in tkMacOSXWm.c.
> And tkUnixWm.c.  And tkWinWm.c

and one in tkUnixSend.c.  That's 5 out of the total 258
Tcl_SetResult calls in Tk. And 2 out of 129 Tcl_SetResult
calls in Tcl (not counting the 2 in tclTest.c, because those
are meant to test the Tcl_SetResult function).

All of them are converted to use Tcl_SetResult(......, TCL_VOLATILE) now.

Regards,
          Jan Nijtmans

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

The patch for TIP #340 is available now, and the description of the
TIP is updated:

    http://sourceforge.net/tracker/index.php?func=detail&aid=2315890&group_id=10894&atid=310894

It contains a solution now for the 'const problem': deprecate Tcl_SetResult for
other values than TCL_STATIC and TCL_VOLATILE. This is a reachable goal, much
more than deprecate all uses of Tcl_SetResult. Actually, I already eliminated
all 'unsafe' Tcl_SetResult calls in Tcl and Tk (except for two in tclTest.c, but
those are meant to test the Tcl_SetResult function by itself).

So, fire away with all remarks, especially about the suggested documentation
change in doc/SetResult.3

Regards,
          Jan Nijtmans

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

[Andreas L. <av...@lo...>]

"Jan Nijtmans" <nij...@us...> wrote:
> The patch for TIP #340 is available now, and the description of the
> TIP is updated:
>     http://sourceforge.net/tracker/index.php?func=detail&aid=2315890&group_id=10894&atid=310894

I think a mention of Tcl_SetObjResult (or other alternatives) should
be added to the Deprecation-block. One could argue that the referral
to the TIP covers it, but I still think it would be a good thing to
have that piece of information immediately at hand without having to
look up the TIP.

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

2008/11/20 Andreas Leitgeb <av...@lo...>:
> "Jan Nijtmans" <nij...@us...> wrote:
>> The patch for TIP #340 is available now, and the description of the
>> TIP is updated:
>>     http://sourceforge.net/tracker/index.php?func=detail&aid=2315890&group_id=10894&atid=310894
>
> I think a mention of Tcl_SetObjResult (or other alternatives) should
> be added to the Deprecation-block. One could argue that the referral
> to the TIP covers it, but I still think it would be a good thing to
> have that piece of information immediately at hand without having to
> look up the TIP.

Agreed, will do that. However, even after the TIP voting, improving
documentation is always a good thing.

Thanks!

Regards,
         Jan Nijtmans

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

2008/11/17 Andreas Leitgeb <av...@lo...>:
> I don't know, if Tcl_SetResult  has still any value apart from
> compatibility.  If it has, then it might be worth a consideration
> adding a variant like "Tcl_SetResultRO"(bikeshed-warning) that will
> take a const char*, and a pointer to a function that in turn will
> also take a const-pointer. That means, there'd also be a
> typedef void (Tcl_FreeProcRO) _ANSI_ARGS_((const char *blockPtr));
> and TCL_STATIC would be eventually be typed as Tcl_FreeProcRO, so
> it could be passed to the const char* taking Tcl_SetResultRO().

A free of a const pointer? That looks wrong to me.

> This appears to me to avoid the "lying to the compiler", by splitting
> up the Tcl_SetResult into one for the const-cases and one for general
> cases.   Anyone who cares about the compiler warnings (hopefully
> including the core) could then use the appropriate one, if creating
> a TclObject and using the "Obj" variant is not desired..

The vast majority of calls use 0 or TCL_VOLATILE, and that's
the CONST case. So we could reserve Tcl_SetResult for
the CONST case and force to use Tcl_SetObjResult in
the other cases. That would already be a lot less work
than modify all Tcl_SetResult calls in the core.

> It may of course be, that if one changes a source at all, he should
> really change it to the "Obj" variant and also make sure that he doesn't
> use interp->result, and also clean up all further legacy-stuff here and
> there in the course... That would of course void my suggestion, but
> possibly be too much work for old code to be reasonable.

Yes, it's a lot of work. Thanks for your reaction.

Regards,
      Jan Nijtmans

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

2008/11/17 Jan Nijtmans <nij...@us...>:
> The vast majority of calls use 0 or TCL_VOLATILE, and that's
> the CONST case. So we could reserve Tcl_SetResult for
> the CONST case and force to use Tcl_SetObjResult in
> the other cases. That would already be a lot less work
> than modify all Tcl_SetResult calls in the core.

In a quick scan, I can only find 4 Tcl_SetResult calls in the
core in which the 3th argument is not 0 or TCL_VOLATILE:
   tclDictObj.c (1 line)
   tclTest.c (2 lines)
   tclThreadTest.c (1 line)
Well, dict is only introduced in Tcl 8.5, and the other two
files are only meant for test cases.

I will modify those 3 files to use Tcl_SetObjResult in stead
of Tcl_Result. That is zero risk and it will ease the discussion
about this subject: For calls in the core the second argument
of Tcl_SetResult is supposed to be a const!

Thanks, Andreas, for triggering this idea. Let's think more
about it.

Regards,
        Jan Nijtmans

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

Note the following code snapshots:

in tclIO.c:
		    /*
		     * Casting away const here is safe because the
		     * TCL_VOLATILE flag guarantees const treatment of the
		     * Posix error string.
		     */

		    Tcl_SetResult(interp, (char *) Tcl_PosixError(interp),
			    TCL_VOLATILE);


Yes, it's OK, but if Tcl_SetResult would have a const char * argument, the
type cast and the comment would not have been neccessary. (But using
Tcl_SetObj Result would be even better here).

in tclDictObj.c:
    /*
     * This next cast is actually OK.
     */

    Tcl_SetResult(interp, (char *) Tcl_HashStats(&dict->table), TCL_DYNAMIC);

No it's not OK! Tcl_HashStats returns an array allocated by either
TclpSysAlloc or ckalloc. In the first case, using TCL_DYNAMIC is simply
wrong. Worse: the signature of Tcl_HashStats is wrong: it should never
have returned a const char*! (I this case, it is actually OK, because
dict hash tables never have the TCL_HASH_KEY_SYSTEM_HASH
flag set, but still the signature and the behavior of Tcl_HashStats
is simply wrong)

Bang! Another bug shot by working on CONST-ness in the core ;-)

Regards,
        Jan Nijtmans

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

[Donald G P. <dg...@ni...>]

Jan Nijtmans wrote:
> ...Worse: the signature of Tcl_HashStats is wrong: it should never
> have returned a const char*! 

Please explain.  Especially in light of the fact that TIP 27 was
the motivation for converting the return value of Tcl_HashStats()
from (char *) to (const char *).

-- 
| Don Porter          Mathematical and Computational Sciences Division |
| don...@ni...             Information Technology Laboratory |
| http://math.nist.gov/~DPorter/                                  NIST |
|______________________________________________________________________|

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

Donald G Porter wrote:

> Jan Nijtmans wrote:
> > ...Worse: the signature of Tcl_HashStats is wrong: it should never
> > have returned a const char*!
>
> Please explain.  Especially in light of the fact that TIP 27 was
> the motivation for converting the return value of Tcl_HashStats()
> from (char *) to (const char *).

Tcl_HashStats is documented as:

| Tcl_HashStats returns a dynamically-allocated string  [...]
| It is the caller’s responsibility to free the result
|                                      ^^^^^^^^^^^^^^
| string by passing it to ckfree.
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So the return value must be a non-const char *.
(Freeing counts as a modification.)


--Joe English

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

2008/11/17 Joe English <jen...@fl...>:
> So the return value must be a non-const char *.
> (Freeing counts as a modification.)

Agreed. The signature is wrong. I plan to fix this in Tcl 8.6, just
by removing the const. That would not cause incompatibility,
because users already were forced to fix type by type casting,
removing the 'const' only relaxes that.

Only, should this be backported to Tcl 8.5 as well? I am
inclined to answer 'Yes'  to that, but how do others feel
about that?

Regards,
           Jan Nijtmans

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

[Donald G P. <dg...@ni...>]

Jan Nijtmans wrote:
> Only, should this be backported to Tcl 8.5 as well? I am
> inclined to answer 'Yes'  to that, but how do others feel
> about that?

 From a practical perspective, it's a big shrug, as in "Don't care,"
since we can count on one hand (one finger?  zero fingers?) the
extensions that have any interest in Tcl_HashStats.  It's a debugging
routine that somehow escaped into the public interface.

Add to that the observation that any code working with the current
implementation pretty much has to work with the revised one too.

-- 
| Don Porter          Mathematical and Computational Sciences Division |
| don...@ni...             Information Technology Laboratory |
| http://math.nist.gov/~DPorter/                                  NIST |
|______________________________________________________________________|

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

[Andreas L. <av...@lo...>]

On Mon, Nov 17, 2008 at 12:16:13PM +0100, Jan Nijtmans wrote:
> 2008/11/17 Andreas Leitgeb <av...@lo...>:
> > typedef void (Tcl_FreeProcRO) _ANSI_ARGS_((const char *blockPtr));
> > and TCL_STATIC would be eventually be typed as Tcl_FreeProcRO, so
> > it could be passed to the const char* taking Tcl_SetResultRO().
> A free of a const pointer? That looks wrong to me.

It's not all that bad, as it would be only used for those freeProcs
that do not really free() the pointer, such as e.g. TCL_STATIC, (which
isn't even a function in the first place).

Since TCL_STATIC and TCL_VOLATILE seem to cover almost all the 
usages, anyway, my original idea of having to change exactly those
to using some Tcl_SetResultRO(...) seems indeed not so good now.

> Yes, it's a lot of work. Thanks for your reaction.

I do not understand all these things that I seem to have triggered,
(especially in your second reply) only so far as that they have little
to do with what I suggested.  If they lead to a better approach, I'm
nevertheless happy :-)

Re: [TCLCORE] TIP #340: Const Qualification of Tcl_SetResult's Argument and -Wwrite-strings

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

Andreas Leitgeb wrote:
> I don't know, if Tcl_SetResult  has still any value apart from
> compatibility.  If it has, then it might be worth a consideration
> adding a variant like "Tcl_SetResultRO"(bikeshed-warning) that will
> take a const char*, and a pointer to a function that in turn will
> also take a const-pointer. That means, there'd also be a
> typedef void (Tcl_FreeProcRO) _ANSI_ARGS_((const char *blockPtr));
> and TCL_STATIC would be eventually be typed as Tcl_FreeProcRO, so
> it could be passed to the const char* taking Tcl_SetResultRO().

A read-only free procedure "void (*Tcl_FreeProcRO)(const char *blockPtr);"
is oxymoronic -- "const" means "I will not modify this argument"
and freeing memory counts as modification.

Simpler: Tcl_SetResultRO wouldn't need or take a third 'freeProc'
argument, it would always just copy the second argument just like
Tcl_SetResult(..., TCL_VOLATILE), and would be typed
void Tcl_SetResultRO(Tcl_Interp *, const char *);


--JE