| Message ID | CAGWvny=3PiUFQQwNKpAxTX1G1tXB8HfoOUwkmRpvdi2WyKhAXg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi David, Looks good to me, but for a small nit: On Tue, Apr 30, 2013 at 7:40 AM, David Edelsohn <dje.gcc@gmail.com> wrote: > + c_fix_arg = "#ifndef NULL\n" > + "#ifdef __cplusplus\n" > + "#ifdef __GNUG__\n" > + "#define NULL\t__null\n" > + "#else\t /* ! __GNUG__ */\n" > + "#define NULL\t0L\n" > + "#endif\t /* __GNUG__ */\n" > + "#else\t /* ! __cplusplus */\n" > + "#define NULL\t((void *)0)\n" > + "#endif\t /* __cplusplus */\n" > + "#endif\t /* !NULL */"; I'd really prefer a "here string": c-fix-arg = <<- _EOF_ #ifndef NULL #ifdef __cplusplus #ifdef __GNUG__ #define NULL __null #else /* ! __GNUG__ */ #define NULL 0L #endif /* __GNUG__ */ #else /* ! __cplusplus */ #define NULL ((void *)0) #endif /* __cplusplus */ #endif /* !NULL */ _EOF_; Unless there is some reason you want to emphasize tabs with the "\t" escapes. I think the "here string" is easier to read.
On Tue, Apr 30, 2013 at 11:28 AM, Bruce Korb <bkorb@gnu.org> wrote: > Hi David, > > Looks good to me, but for a small nit: > Unless there is some reason you want to emphasize tabs with the "\t" escapes. > I think the "here string" is easier to read. I coped c_fix_arg from openbsd_null_definition fix. I don't know if two different formats for similar fixes would be confusing, but I do not have a problem using the EOF version. Do you have any comment about the change to void_null fix? Thanks, David
Hi, On Tue, Apr 30, 2013 at 9:42 AM, David Edelsohn <dje.gcc@gmail.com> wrote: > I coped c_fix_arg from openbsd_null_definition fix. I don't know if > two different formats for similar fixes would be confusing, but I do > not have a problem using the EOF version. I haven't reviewed all patches and I don't always raise nits. > Do you have any comment about the change to void_null fix? By analogy, I'd prefer the EOF version there, too. But ultimately, it works and is not exactly illegible, so I have no serious objection to the fix. It would be nice if it were combinable, but doing so is tricky. I'm guessing that the fix on AIX is separated because the headers contain "__cplusplus" and that is used as an exclusion test for the "void_null" fix? Otherwise this: select = "^#[ \t]*define[ \t]+NULL[ \t]+(" "\\(\\(void[ \t]*\\*\\)0\\)" "|" "\\(*0L*\\)*" ")"; would allow void_null to work for you -- perhaps by using %1 somewhere in the replacement "c-fix-arg". Cheers - Bruce
On Tue, Apr 30, 2013 at 1:09 PM, Bruce Korb <bkorb@gnu.org> wrote: > By analogy, I'd prefer the EOF version there, too. But ultimately, > it works and is not exactly illegible, so I have no serious objection > to the fix. It would be nice if it were combinable, but doing so is > tricky. I'm guessing that the fix on AIX is separated because the > headers contain "__cplusplus" and that is used as an exclusion test > for the "void_null" fix? Otherwise this: > > select = "^#[ \t]*define[ \t]+NULL[ \t]+(" > "\\(\\(void[ \t]*\\*\\)0\\)" "|" "\\(*0L*\\)*" ")"; > > would allow void_null to work for you -- perhaps by using %1 somewhere > in the replacement "c-fix-arg". Yes, I created a separate fix because the void_null fix has additional selection details that I did not want to change and possibly break other platforms. Thanks, David
Index: inclhack.def =================================================================== --- inclhack.def (revision 198437) +++ inclhack.def (working copy) @@ -617,6 +617,31 @@ }; /* + * Fix AIX definition of NULL for G++. + */ +fix = { + hackname = aix_null; + mach = "*-*-aix*"; + files = curses.h, dbm.h, locale.h, stdio.h, stdlib.h, string.h, + time.h, unistd.h, wchar.h, sys/dir.h, sys/param.h, sys/types.h; + bypass = __null; + select = "#define[ \t]+NULL[ \t]+\\(*0L*\\)*"; + c_fix = format; + c_fix_arg = "#ifndef NULL\n" + "#ifdef __cplusplus\n" + "#ifdef __GNUG__\n" + "#define NULL\t__null\n" + "#else\t /* ! __GNUG__ */\n" + "#define NULL\t0L\n" + "#endif\t /* __GNUG__ */\n" + "#else\t /* ! __cplusplus */\n" + "#define NULL\t((void *)0)\n" + "#endif\t /* __cplusplus */\n" + "#endif\t /* !NULL */"; + test_text = "# define\tNULL \t(0L) /* typed NULL */"; +}; + +/* * pthread.h on AIX defines PTHREAD_ONCE_INIT, PTHREAD_MUTEX_INITIALIZER, * PTHREAD_COND_INITIALIZER and PTHREAD_RWLOCK_INITIALIZER without enough * braces. @@ -4552,23 +4577,24 @@ */ fix = { hackname = void_null; - files = curses.h; - files = dbm.h; - files = locale.h; - files = stdio.h; - files = stdlib.h; - files = string.h; - files = time.h; - files = unistd.h; - files = sys/dir.h; - files = sys/param.h; - files = sys/types.h; + files = curses.h, dbm.h, locale.h, stdio.h, stdlib.h, string.h, + time.h, unistd.h, sys/dir.h, sys/param.h, sys/types.h; /* avoid changing C++ friendly NULL */ bypass = __cplusplus; bypass = __null; select = "^#[ \t]*define[ \t]+NULL[ \t]+\\(\\(void[ \t]*\\*\\)0\\)"; c_fix = format; - c_fix_arg = "#define NULL 0"; + c_fix_arg = "#ifndef NULL\n" + "#ifdef __cplusplus\n" + "#ifdef __GNUG__\n" + "#define NULL\t__null\n" + "#else\t /* ! __GNUG__ */\n" + "#define NULL\t0L\n" + "#endif\t /* __GNUG__ */\n" + "#else\t /* ! __cplusplus */\n" + "#define NULL\t((void *)0)\n" + "#endif\t /* __cplusplus */\n" + "#endif\t /* !NULL */"; test_text = "# define\tNULL \t((void *)0) /* typed NULL */"; };
AIX headers do not have a complete definition of NULL and the GCC build complains about "missing sentinel". The appended patch substitutes a more complete definition of NULL that satisfies GCC/G++. I also updated the void_null fix, which was discussed in 2011 to substitute a more complete definition of NULL. void_null no longer triggers on AIX because of the "__cplusplus" bypass, which appears in AIX header files. I can omit void_null from the patch or remove the fix completely if it no longer is applicable to any target. (Yes, I will update fixinclude tests, but I first want agreement on the fix itself.) Bootstrapped on powerpc-ibm-aix7.1.0.0. Thanks, David * inclhack.def (aix_null): New. (void_null): Substitute newer definition of NULL. * fixincl.x: Regenerate.