From: Keith Seitz <keiths@redhat.com>
To: Roland Schwingel <roland@onevision.com>
Cc: insight@sourceware.org
Subject: Re: [PATCH] libgui: Make it compileable on windows with recent gcc versions and also on win64
Date: Mon, 04 Jun 2012 01:13:00 -0000 [thread overview]
Message-ID: <4FCC0BAB.9000402@redhat.com> (raw)
In-Reply-To: <4FBF744B.9050305@onevision.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
next prev parent reply other threads:[~2012-06-04 1:13 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-25 12:00 Roland Schwingel
2012-06-04 1:13 ` Keith Seitz [this message]
-- strict thread matches above, loose matches on Subject: below --
2012-08-08 13:37 Roland Schwingel
2012-03-23 10:22 Roland Schwingel
2012-05-24 21:35 ` Keith Seitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FCC0BAB.9000402@redhat.com \
--to=keiths@redhat.com \
--cc=insight@sourceware.org \
--cc=roland@onevision.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).