Tcl Source Code

View Ticket
Login
Ticket UUID: 509340
Title: ClientData set to int* not void*
Type: Patch Version: None
Submitter: davygrvy Created on: 2002-01-27 23:45:10
Subsystem: 55. Other Tools Assigned To: mdejong
Priority: 5 Medium Severity:
Status: Closed Last Modified: 2002-07-20 18:00:30
Resolution: Rejected Closed By: davygrvy
    Closed on: 2002-07-20 11:00:30
Description:
With the MS compiler, the following is allowed without 
casting even with langauge extensions turned off (-Za).

/*#include "tcl.h"*/
#ifdef __STDC__
#   if defined(__STDC_VERSION__) && __STDC_VERSION__ 
== 199409L
#pragma message ("*** ISO C and AM1 complient")
#   else
#pragma message ("*** ISO C, but not AM1 
complient")
#   endif
#else
#   pragma message ("*** Not ISO C complient")
#endif

void main (void)
{
    /*ClientData someTypeLessPointer;*/
    void *someTypeLessPointer;
    struct {
__int64 a;
char b;
    } someStruct;

    someTypeLessPointer = &someStruct;
}

Placing a typed pointer into a typeless pointer is 
allowed.  The typedef for ClientData should be void*, 
not int* under this compiler, IMO.

Notice the test for __STDC__ is a failure under this 
compiler, too.  __STDC__ is only defined when the -Za 
switch is used.  I'm at a loss to understand what the 
complience of the compiler really is, but getting back 
to the patch, a void* is legal, so let's use it.
User Comments: davygrvy added on 2002-07-20 18:00:30:
Logged In: YES 
user_id=7549

already fixed by another patch.

nobody added on 2002-05-28 21:30:48:
Logged In: NO 

Thinking about it, my previous proposal is probably too
drastic - given that Jeff defends the sources like a lioness
would defend her babies :)
I just had a look at the sources of Tcl8.4 and noted that
the test now looks like 
#if defined(__STDC__) || defined(__cplusplus) ||
defined(__BORLANDC__) So the logical thing to do would be to
explicitely add VC++ there, too, via defined(MSCVER) - or
similar.Regards
Helmut

nobody added on 2002-05-28 19:39:22:
Logged In: NO 

Currently ClientData is defined as follows (from tcl.h)
#   if defined(__STDC__) || defined(__cplusplus)
    typedef void *ClientData;
#   else
    typedef int *ClientData;
#   endif /* __STDC__ */ 
With the Borland compiler (and apparently with VC++, too)
this will result in" typedef int *ClientData", leading to a
lot of otherwise unnecessary typecasts.

Explanation:
For the Borland compiler __STDC__ means *strict* compliance
to ANSI C (you can set this mode with the -A flag). With
this flag turned on, however, Tcl won't compile - some
Windows header files containing C++ style (//) comments. I
tend to think that this is a sensible thing to do (you want
ANSI C, you get - strict - ANSI C), but this is just opinion
- we got a problem to solve.

The criterion used above
    #   if defined(__STDC__) || defined(__cplusplus)
assumes this scenario:
- pre-ANSI C
- ANSI C or
- C++

However this does not describe the actual situation, where
we have sort of a post-ANSI period: C compilers adhering to
the standard but allowing extensions, too (like C++
comments). (And differing of course in whether they #define
__STDC__ for these extensions or not.).
I would therefore suggest that we change the criterion.
After all, all we want to know is whether the compiler knows
the 'void' key word. We don't really care (at this point)
whether its internal logic defines __STDC__. So my
suggestion is to add a 
   #define KNOSW_VOID
and use this for the typedef of ClientData

#   ifdef KNOWS_VOID
    typedef void *ClientData;
#   else
    typedef int *ClientData;
 #   endif /* KNOWS_VOID */

I assume that all current compilers support 'void', so we
probably don't even need this check. If, however, it turns
out that there is a compiler which barks on 'void', we could
add stuff to handle just this compiler.

#define KNOWS_VOID

#if defined SomeWeirdCompiler
#undef KNOWS_VOID
#endif
In any case, we should change the criterion in some way,
because it doesn't handle the actual situation.

Just my 0.02
Helmut Giese

hobbs added on 2002-03-05 12:41:31:
Logged In: YES 
user_id=72656

Can someone tell me why this needs to be applied?  I still 
fail to see a problem with the current sources.

davygrvy added on 2002-03-05 12:07:51:
Logged In: YES 
user_id=7549

MinGW gets a void*, why can't VC++?  Notice -D__STDC__=1 
below.  VC++ doesn't set that.  It only sets it under 
strict -Za, which not having it defined doesn't mean less 
than, which it isn't.  That's like if gcc only set -
D__STDC__=1 *only* when -pedantic -ansi was used.  The 
current logic of looking at the __STDC__ #define is 
missleading under VC++.

 c:\dev\mingw\bin\..\lib\gcc-lib\mingw32\2.95.3-5\cpp0.exe -
lang-c -v -iprefix c:\dev\mingw\bin\../lib/gcc-
lib/mingw32/2.95.3-5/ -D__GNUC__=2 -D__GNUC_MINOR__=95 -
Di386 -D_WIN32 -DWIN32 -D__WIN32__ -D__MINGW32__=1.0 -
D__MSVCRT__ -DWINNT -D_X86_=1 -D__STDC__=1 -
D__stdcall=__attribute__((__stdcall__)) -D_stdcall=__attrib
ute__((__stdcall__)) -D__cdecl=__attribute__((__cdecl__)) -
D__declspec(x)=__attribute__((x)) -D__i386__ -D_WIN32 -
D__WIN32__ -D__WIN32__ -D__MINGW32__=1.0 -D__MSVCRT__ -
D__WINNT__ -D_X86_=1 -D__STDC__=1 -D__stdcall=__attribute__
((__stdcall__)) -D___stdcall__=__attribute__
((__stdcall__)) -D__cdecl=__attribute__((__cdecl__)) -
D__declspec(x)=__attribute__((x)) -D__i386 -D__WIN32 -
D__WINNT -D___stdcall=__attribute__((__stdcall__)) -Asystem
(winnt) -Acpu(i386) -Amachine(i386) -remap -Acpu(i386) -
Amachine(i386) -Di386 -D__i386 -D__i386__ --help help-dummy 
C:\DOCUME~1\daveg\LOCALS~1\Temp\ccGGaaaa.i

davygrvy added on 2002-02-26 03:44:29:
Logged In: YES 
user_id=7549

This is not the source, it's the typedef.  It doesn't make 
sense.  Why on earth is an int* used when a void* can be 
and is easy to work with.  I hate casting when it isn't 
needed.  VC++ doesn't need to cast, as it understand what a 
void* is.

What does:

#if defined(__STDC__) || defined(__cplusplus)

mean to you?  Does it mean your compiler doesn't understand 
a void*, or we are going to force you to cast in both 
directions even if your compiler doesn't need to?

hobbs added on 2002-02-26 02:02:47:
Logged In: YES 
user_id=72656

I fail to see a problem stated.  If it ain't broke, don't 
fix it.  Unless there is a clear problem with the current 
source, let's not change things.

davygrvy added on 2002-02-23 14:53:30:
Logged In: YES 
user_id=7549

forget "some systems".  This is Win32 with MSVC as the 
compiler.

What is that test asking?  Is it asking are you strict ANSI 
or ANSI compilent as oppossed to pre-ANSI?

#ifndef _CLIENTDATA
#   if defined(__STDC__) || defined(__cplusplus) || defined
(__BORLANDC__)
typedef void *ClientData;
#   else
typedef int *ClientData;
#   endif /* __STDC__ */
#   define _CLIENTDATA
#endif

More flexible?  How so?  sizeof(*(ClientData))?  What good 
is that, not that it would work?

I do most of my work in C++ and never had to cast a typed 
pointer to a ClientData.  Coming out of a ClientData to a 
typed pointer, of course yes, I had to cast.  Then I do 
some work in C, and I get this damn cast shit going into a 
ClientData.  But its still legal in C.  Typed pointer to a 
void pointer without a cast is legal in this compiler for 
C.  If this is the case, and is fine in C++ according to 
the existing logic, don't force an int* when it isn't 
needed, please.  There are users of the Tcl API besides the 
core itself where old-fashioned seems to always take 
precidence over advances.  Just because its there, and has 
been for a while, doesn't mean its right by only it's 
existence.  Please question the meaning of the preprocessor 
logic.  It doesn't make sense for this compiler.

#if !defined(__STDC__) and !defined(__cplusplus) == 1
does that mean a void* isn't legal?  But in the case for 
defined(_MSC_VER), it is true, though.

Is that test testing a compiler feature or forcing an 
interface?  If it's forcing an interface, why?  It gets in 
my way as a user of the API.

hobbs added on 2002-02-23 14:04:17:
Logged In: YES 
user_id=72656

I'm not sure that I see this is really a problem in the 
sources though ... also int* gives more flexibility than 
void* on some systems.

davygrvy added on 2002-01-28 07:07:31:
Logged In: YES 
user_id=7549

In this case, I guess "not ISO C" means greater than 
instead of "sub ISO C"

davygrvy added on 2002-01-28 06:45:14:

File Added - 16878: diff.txt

Attachments: