Re: [PATCH] libgui: Make it compileable on windows with recent gcc versions and also on win64

[Keith Seitz <keiths@redhat.com>]

Thanks for the patch. I only have a (very) minor few nits...

On 05/25/2012 05:00 AM, Roland Schwingel wrote:
>
> diff -ruN libgui_orig/src/subcommand.c libgui/src/subcommand.c
> --- libgui_orig/src/subcommand.c	2001-09-09 00:34:47.000000000 +0200
> +++ libgui/src/subcommand.c	2012-02-24 13:53:50.760585300 +0100
> @@ -34,7 +34,7 @@
>      table.  */
>   static int
>   subcommand_implementation (ClientData cd, Tcl_Interp *interp,
> -			   int argc, char *argv[])
> +			   int argc,CONST84 char *argv[])

Since this is still our code, I would prefer we stick to GNU as much as 
possible/feasible. Please watch the spaces after ','. There are several 
places like this.

>   {
>     struct subcommand_clientdata *data = (struct subcommand_clientdata *) cd;
>     const struct ide_subcommand_table *commands = data->commands;
> @@ -90,7 +90,7 @@
>
>   /* Define a command with subcommands.  */
>   int
> -ide_create_command_with_subcommands (Tcl_Interp *interp, char *name,
> +ide_create_command_with_subcommands (Tcl_Interp *interp, const char *name,
>   				     const struct ide_subcommand_table *table,
>   				     ClientData subdata,
>   				     Tcl_CmdDeleteProc *delete)

This appears to be > 80 characters and needs line-wrapping. Several 
places where this happens, too.

> diff -ruN libgui_orig/src/tclmsgbox.c libgui/src/tclmsgbox.c
> --- libgui_orig/src/tclmsgbox.c	2001-09-09 00:34:47.000000000 +0200
> +++ libgui/src/tclmsgbox.c	2012-02-24 13:33:44.464962700 +0100
> @@ -4,11 +4,11 @@
>
>   #ifdef _WIN32
>
> +#include<windows.h>
> +
>   #include<tcl.h>
>   #include<tk.h>
>
> -#include<windows.h>
> -
>   /* FIXME: We use some internal Tcl and Tk Windows stuff.  */
>   #include<tkWinInt.h>
>
> @@ -96,7 +96,7 @@
>     class.hInstance = TclWinGetTclInstance();
>     class.hbrBackground = NULL;
>     class.lpszMenuName = NULL;
> -  class.lpszClassName = "ide_messagebox";
> +  class.lpszClassName = TEXT("ide_messagebox");
>     class.lpfnWndProc = msgbox_wndproc;
>     class.hIcon = NULL;
>     class.hCursor = NULL;

GNU wants a space before '(' (when possible). Needs addressing in 
several places. [I realize this isn't very consistent across libgui, but 
let's just give it a try.]

> diff -ruN libgui_orig/src/tclwinfont.c libgui/src/tclwinfont.c
> --- libgui_orig/src/tclwinfont.c	2001-09-09 00:34:47.000000000 +0200
> +++ libgui/src/tclwinfont.c	2012-02-24 13:42:41.393703800 +0100
> @@ -154,7 +154,7 @@
>       }
>
>     oldMode = Tcl_SetServiceMode(TCL_SERVICE_ALL);
> -  if (! ChooseFont (&cf))
> +  if (! ChooseFontA (&cf))
>       {
>         DWORD code;

Might as well take this opportunity to make this more GNU-compatible. 
Please use "!ChooseFontA".

> diff -ruN libgui_orig/src/tclwinprint.c libgui/src/tclwinprint.c
> --- libgui_orig/src/tclwinprint.c	2001-09-09 00:34:47.000000000 +0200
> +++ libgui/src/tclwinprint.c	2012-02-24 13:57:12.703864500 +0100
> diff -ruN libgui_orig/src/tkTable.c libgui/src/tkTable.c
> --- libgui_orig/src/tkTable.c	2001-09-09 00:34:47.000000000 +0200
> +++ libgui/src/tkTable.c	2012-02-24 15:25:18.870312200 +0100
> @@ -1041,7 +1037,7 @@
>       /* Do the configuration */
>       argv = StringifyObjects(objc, objv);
>       result = Tk_ConfigureWidget(interp, tablePtr->tkwin, tableSpecs,
> -	    objc, argv, (char *) tablePtr, flags);
> +	    objc, (CONST84 char **)argv, (char *) tablePtr, flags);

Please use "(CONST84 char **) argv". Another GNU-ism.

>       ckfree((char *) argv);
>       if (result != TCL_OK) {
>   	return TCL_ERROR;

> diff -ruN libgui_orig/src/tkTable.h libgui/src/tkTable.h
> --- libgui_orig/src/tkTable.h	2001-09-09 00:34:47.000000000 +0200
> +++ libgui/src/tkTable.h	2012-03-22 15:50:05.366470500 +0100
> @@ -15,9 +15,26 @@
>   #ifndef_TKTABLE_H_
>   #define_TKTABLE_H_
>
> +#ifdef WIN32
> +#   define WIN32_LEAN_AND_MEAN
> +#   include<windows.h>
> +#   undef WIN32_LEAN_AND_MEAN
> +/* VC++ has an entry point called DllMain instead of DllEntryPoint */
> +#   if defined(_MSC_VER)
> +#	define DllEntryPoint DllMain
> +#   endif
> +#endif

I wonder if this will conflict with Cygwin... Honestly, I don't know. 
I'm still having big problems with Cygwin builds.

> @@ -631,6 +638,31 @@
>   		(rowPtr), (colPtr))
>
>   /*
> + * Macros used to cast between pointers and integers (e.g. when storing an int
> + * in ClientData), on 64-bit architectures they avoid gcc warning about "cast
> + * to/from pointer from/to integer of different size".
> + */
> +
> +#if !defined(INT2PTR)&&  !defined(PTR2INT)
> +#   if defined(HAVE_INTPTR_T) || defined(intptr_t)
> +#  define INT2PTR(p) ((void *)(intptr_t)(p))
> +#  define PTR2INT(p) ((int)(intptr_t)(p))
> +#   else
> +#  define INT2PTR(p) ((void *)(p))
> +#  define PTR2INT(p) ((int)(p))
> +#   endif
> +#endif
> +#if !defined(UINT2PTR)&&  !defined(PTR2UINT)
> +#   if defined(HAVE_UINTPTR_T) || defined(uintptr_t)
> +#  define UINT2PTR(p) ((void *)(uintptr_t)(p))
> +#  define PTR2UINT(p) ((unsigned int)(uintptr_t)(p))
> +#   else
> +#  define UINT2PTR(p) ((void *)(p))
> +#  define PTR2UINT(p) ((unsigned int)(p))
> +#   endif
> +#endif

I'm not going to force this file to the GNU Coding Standard. Just please 
be consistent with the surrounding code. For example, "!defined(INT2PTR) 
&& !defined(PTR2INT)" seems more natural.

> diff -ruN libgui_orig/src/tkWinPrintText.c libgui/src/tkWinPrintText.c
> --- libgui_orig/src/tkWinPrintText.c	2001-09-09 00:34:48.000000000 +0200
> +++ libgui/src/tkWinPrintText.c	2012-03-22 13:56:42.037502500 +0100
> +#if (TCL_MAJOR_VERSION>= 8)&&  (TCL_MINOR_VERSION>= 5)
> +    numLines = TkBTreeNumLines(textPtr->sharedTextPtr->tree,textPtr);
> +    TkTextMakeByteIndex(textPtr->sharedTextPtr->tree, textPtr, 0, 0,&first);
> +    TkTextMakeByteIndex(textPtr->sharedTextPtr->tree, textPtr, numLines, 100,&last);	
> +    TkTextChanged(textPtr->sharedTextPtr, textPtr,&first,&last);
> +#elif (TCL_MAJOR_VERSION>= 8)&&  (TCL_MINOR_VERSION>= 1)
>       numLines = TkBTreeNumLines(textPtr->tree);
> -#if (TCL_MAJOR_VERSION>= 8)&&  (TCL_MINOR_VERSION>= 1)
>       TkTextMakeByteIndex(textPtr->tree, 0, 0,&first);
>       TkTextMakeByteIndex(textPtr->tree, numLines, 100,&last);
> +    TkTextChanged(textPtr,&first,&last);
>   #else
> +    numLines = TkBTreeNumLines(textPtr->tree);
>       TkTextMakeIndex(textPtr->tree, 0, 0,&first);
>       TkTextMakeIndex(textPtr->tree, numLines, 100,&last);
> -#endif
>       TkTextChanged(textPtr,&first,&last);
> -
> +#endif
>       /*
>        * Set the display info flag to out-of-date.
>        */

I'd like to see spaces before/after "&&", "||", ">", "<", ">=", etc. 
This happens in a few places.

Wow, that was one major patch. In the end, I must rely on you and any 
kind Windows/MinGW-savvy readers to verify the Windows bits. I compiled 
it all on a Windows VM that I have, but I don't regularly use that 
platform for any serious work.

Ok with those changes.

I apologize for the delay in getting this reviewed. It's been anything 
but timely. Thanks a *bunch* for your patience!

Keith