Thread: Patch: Remove gcc dependency in definition of inline functions
Hi, The attached patch is offered for the 2010-01 commitfest. It's also available in my git repository in the "submitted" branch: http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted In palloc.h and pg_list.h, some inline functions are defined if allowed by the compiler. Previously the test was gcc-specific. This patch enables inlining on more platforms, relying on the Autoconf-generated "configure" script to determine whether the compiler supports inline functions. Depending on the compiler, the keyword for defining an inline function might be spelled as inline, __inline, or __inline__, or none of these. "configure" finds out, and except in the first case, puts a suitable "#define inline" into pg_config.h. This hasn't changed. What's new is that "configure" will add "#define HAVE_INLINE 1" into pg_config.h if it detects that the compiler accepts inline function definitions. palloc.h and pg_list.h will condition their inline function definitions on HAVE_INLINE instead of the gcc-specific __GNUC__. After applying the patch, run these commands to update the configure script and pg_config.h.in: autoconf; autoheader (Does anybody still use a C compiler that doesn't support inline functions?) Regards, ... kurt Remove gcc dependency in definition of "inline" functions. In palloc.h and pg_list.h, some inline functions are defined if allowed by the compiler. Previously the test was gcc-specific. Now the test is platform independent, relying on the Autoconf- generated "configure" script to determine whether the compiler supports inline functions. Depending on the compiler, the keyword for defining an inline function might be spelled as inline, __inline, or __inline__, or none of these. "configure" finds out, and except in the first case, puts a suitable "#define inline" into pg_config.h. This hasn't changed. What's new is that "configure" now adds "#define HAVE_INLINE 1" into pg_config.h upon finding that the compiler accepts inline function definitions. palloc.h and pg_list.h now condition their inline function definitions on HAVE_INLINE instead of the gcc-specific __GNUC__. --- configure.in | 4 ++++ src/backend/nodes/list.c | 10 ++++------ src/backend/utils/mmgr/mcxt.c | 9 ++++----- src/include/nodes/pg_list.h | 4 ++-- src/include/utils/palloc.h | 8 ++++---- 5 files changed, 18 insertions(+), 17 deletions(-) --- Kurt Harriman <harriman@acm.org> 2009-11-29 08:22:05 -0800 diff --git a/configure.in b/configure.in index 612e843..467f40d 100644 --- a/configure.in +++ b/configure.in @@ -1105,6 +1105,10 @@ PGAC_STRUCT_SOCKADDR_STORAGE PGAC_STRUCT_SOCKADDR_STORAGE_MEMBERS PGAC_STRUCT_ADDRINFO +if test "$ac_cv_c_inline" != no; then + AC_DEFINE(HAVE_INLINE, 1, [Define to 1 if inline functions are allowed in C]) +fi + AC_CHECK_TYPES([struct cmsgcred, struct fcred, struct sockcred], [], [], [#include <sys/param.h> #include <sys/types.h> diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c index 1ba85f4..66fa3d6 100644 --- a/src/backend/nodes/list.c +++ b/src/backend/nodes/list.c @@ -1224,12 +1224,10 @@ list_copy_tail(List *oldlist, int nskip) } /* - * When using non-GCC compilers, we can't define these as inline - * functions in pg_list.h, so they are defined here. - * - * TODO: investigate supporting inlining for some non-GCC compilers. + * pg_list.h defines inline versions of this function if allowed by the + * compiler; in which case the definitions below are skipped. */ -#ifndef __GNUC__ +#ifndef HAVE_INLINE ListCell * list_head(List *l) @@ -1248,7 +1246,7 @@ list_length(List *l) { return l ? l->length : 0; } -#endif /* ! __GNUC__ */ +#endif /* ! HAVE_INLINE */ /* * Temporary compatibility functions diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 4939046..2c5ddbb 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -628,11 +628,10 @@ repalloc(void *pointer, Size size) * MemoryContextSwitchTo * Returns the current context; installs the given context. * - * This is inlined when using GCC. - * - * TODO: investigate supporting inlining for some non-GCC compilers. + * palloc.h defines an inline version of this function if allowed by the + * compiler; in which case the definition below is skipped. */ -#ifndef __GNUC__ +#ifndef HAVE_INLINE MemoryContext MemoryContextSwitchTo(MemoryContext context) @@ -645,7 +644,7 @@ MemoryContextSwitchTo(MemoryContext context) CurrentMemoryContext = context; return old; } -#endif /* ! __GNUC__ */ +#endif /* ! HAVE_INLINE */ /* * MemoryContextStrdup diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index 04da862..4e3e08d 100644 --- a/src/include/nodes/pg_list.h +++ b/src/include/nodes/pg_list.h @@ -74,7 +74,7 @@ struct ListCell * arguments. Therefore, we implement them using GCC inline functions, * and as regular functions with non-GCC compilers. */ -#ifdef __GNUC__ +#ifdef HAVE_INLINE static __inline__ ListCell * list_head(List *l) @@ -98,7 +98,7 @@ list_length(List *l) extern ListCell *list_head(List *l); extern ListCell *list_tail(List *l); extern int list_length(List *l); -#endif /* __GNUC__ */ +#endif /* HAVE_INLINE */ /* * NB: There is an unfortunate legacy from a previous incarnation of diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h index e504ffa..d9eac0a 100644 --- a/src/include/utils/palloc.h +++ b/src/include/utils/palloc.h @@ -72,11 +72,11 @@ extern void *repalloc(void *pointer, Size size); /* * MemoryContextSwitchTo can't be a macro in standard C compilers. - * But we can make it an inline function when using GCC. + * But we can make it an inline function if the compiler supports it. */ -#ifdef __GNUC__ +#ifdef HAVE_INLINE -static __inline__ MemoryContext +static inline MemoryContext MemoryContextSwitchTo(MemoryContext context) { MemoryContext old = CurrentMemoryContext; @@ -87,7 +87,7 @@ MemoryContextSwitchTo(MemoryContext context) #else extern MemoryContext MemoryContextSwitchTo(MemoryContext context); -#endif /* __GNUC__ */ +#endif /* HAVE_INLINE */ /* * These are like standard strdup() except the copied string is
Kurt Harriman <harriman@acm.org> writes:
> (Does anybody still use a C compiler that doesn't support
> inline functions?)
The question isn't so much that, it's whether the compiler supports
inline functions with the same behavior as gcc. At minimum that
would require* not generating extra copies of the function* not whining about unreferenced static functions
How many compilers have you tested this patch against? Which ones
does it actually offer any benefit for?
regards, tom lane
On 11/29/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kurt Harriman <harriman@acm.org> writes: > > (Does anybody still use a C compiler that doesn't support > > inline functions?) +1 for modern C. > The question isn't so much that, it's whether the compiler supports > inline functions with the same behavior as gcc. At minimum that > would require > * not generating extra copies of the function > * not whining about unreferenced static functions > How many compilers have you tested this patch against? Which ones > does it actually offer any benefit for? Those are not correctness problems. Compilers with non-optimal or missing 'inline' do belong together with compilers without working int64. We may spend some effort to be able to compile on them, but they should not affect our coding style. 'static inline' is superior to macros because of type-safety and side-effects avoidance. I'd suggest event removing any HAVE_INLINE ifdefs and let autoconf undef the 'inline' if needed. Less duplicated code to maintain. The existence of compilers in active use without working 'inline' seems quite hypothetical these days. -- marko
Marko Kreen wrote: > On 11/29/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Kurt Harriman <harriman@acm.org> writes: > > > (Does anybody still use a C compiler that doesn't support > > > inline functions?) > > +1 for modern C. > > > The question isn't so much that, it's whether the compiler supports > > inline functions with the same behavior as gcc. At minimum that > > would require > > * not generating extra copies of the function > > * not whining about unreferenced static functions > > How many compilers have you tested this patch against? Which ones > > does it actually offer any benefit for? > > Those are not correctness problems. Compilers with non-optimal or > missing 'inline' do belong together with compilers without working int64. > We may spend some effort to be able to compile on them, but they > should not affect our coding style. > > 'static inline' is superior to macros because of type-safety and > side-effects avoidance. I'd suggest event removing any HAVE_INLINE > ifdefs and let autoconf undef the 'inline' if needed. Less duplicated > code to maintain. The existence of compilers in active use without > working 'inline' seems quite hypothetical these days. I thought one problem was that inline is a suggestion that the compiler can ignore, while macros have to be implemented as specified. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On mån, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote: > I thought one problem was that inline is a suggestion that the compiler > can ignore, while macros have to be implemented as specified. Sure, but one could argue that a compiler that doesn't support inline usefully is probably not the sort of compiler that you use for compiling performance-relevant software anyway. We can support such systems in a degraded way for historical value and evaluation purposes as long as it's pretty much free, like we support systems without working int8.
Peter Eisentraut wrote: > On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote: > > I thought one problem was that inline is a suggestion that the compiler > > can ignore, while macros have to be implemented as specified. > > Sure, but one could argue that a compiler that doesn't support inline > usefully is probably not the sort of compiler that you use for compiling > performance-relevant software anyway. We can support such systems in a > degraded way for historical value and evaluation purposes as long as > it's pretty much free, like we support systems without working int8. The issue is that many compilers will take "inline" as a suggestion and decide if it is worth-while to inline it --- I don't think it is inlined unconditionally by any modern compilers. Right now we think we are better at deciding what should be inlined than the compiler --- of course, we might be wrong, and it would be good to performance test this. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On 11/30/09, Bruce Momjian <bruce@momjian.us> wrote: > Peter Eisentraut wrote: > > On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote: > > > I thought one problem was that inline is a suggestion that the compiler > > > can ignore, while macros have to be implemented as specified. > > > > Sure, but one could argue that a compiler that doesn't support inline > > usefully is probably not the sort of compiler that you use for compiling > > performance-relevant software anyway. We can support such systems in a > > degraded way for historical value and evaluation purposes as long as > > it's pretty much free, like we support systems without working int8. > > > The issue is that many compilers will take "inline" as a suggestion and > decide if it is worth-while to inline it --- I don't think it is inlined > unconditionally by any modern compilers. > > Right now we think we are better at deciding what should be inlined than > the compiler --- of course, we might be wrong, and it would be good to > performance test this. Note - my proposal would be to get rid of HAVE_INLINE, which means we are already using inline functions unconditionally on platforms that matter (gcc). Keeping duplicate code for obsolete compilers is pointless. I'm not suggesting converting all existing macros to inline. This can happen slowly, and where it brings benefit (short but multi-arg are probably most worthwhile to convert). Also new macros are better done as inlines. About uninlining - usually if the compiler decides to uninline, it is probably right anyway. The prime example would be Linux kernel where the 'inline' is used quite a lot as go-faster-magic-dust on totally random functions. (In .c files, not talking about headers) Most compilers (gcc, vc, icc) support also force-inlining, which is not taken as hint but command. If you want to be on safe side, you could define 'inline' as force-inline on compilers that support that. -- marko
Marko Kreen wrote: > Note - my proposal would be to get rid of HAVE_INLINE, which > means we are already using inline functions unconditionally > on platforms that matter (gcc). Keeping duplicate code > for obsolete compilers is pointless. > Microsoft C doesn't matter? I seem to remember that when the Win32 version became available it actually increased the number of people trying postgres rather dramatically. Did that count for nothing?
James Mansion <james@mansionfamily.plus.com> writes:
> Marko Kreen wrote:
>> Note - my proposal would be to get rid of HAVE_INLINE, which
>> means we are already using inline functions unconditionally
>> on platforms that matter (gcc). Keeping duplicate code
>> for obsolete compilers is pointless.
> Microsoft C doesn't matter?
Breaking compilers that don't have inline at all isn't happening;
it wouldn't buy us anything much anyway. The debate here is about
how much we can assume about the behavior of compilers that do
recognize the keyword. In particular, do they behave sensibly
when finding an unreferenced static inline function, which is what
would occur in many modules if we allow them to see inline functions
in headers.
regards, tom lane
On 12/2/09, James Mansion <james@mansionfamily.plus.com> wrote: > Marko Kreen wrote: > > > Note - my proposal would be to get rid of HAVE_INLINE, which > > means we are already using inline functions unconditionally > > on platforms that matter (gcc). Keeping duplicate code > > for obsolete compilers is pointless. > > > > > Microsoft C doesn't matter? > > I seem to remember that when the Win32 version became available it actually > increased the > number of people trying postgres rather dramatically. Did that count for > nothing? The "(gcc)" above meant the inline functions are already used with gcc. I have no reason to think Microsoft's inlining works worse than gcc's. IOW - if the compiler does not support 'static inline' we should fall back to plain 'static' functions, instead maintaining duplicate macros. Such compilers would take a efficiency hit, but as they are practically non-existent they dont matter. Microsoft C does support inline, so it would not be affected. -- marko
Tom Lane wrote: > Kurt Harriman <harriman@acm.org> writes: >> (Does anybody still use a C compiler that doesn't support >> inline functions?) > The question isn't so much that, it's whether the compiler supports > inline functions with the same behavior as gcc. With this patch, if the compiler doesn't accept the "inline" keyword (or __inline or __inline__), then HAVE_INLINE remains undefined, and the out-of-line definitions are used. So, these concerns would be applicable in case there is a compiler which accepts the keyword but ignores it, thus fooling "configure". (Also these concerns become applicable if we eliminate the out-of-line definitions altogether as some have suggested.) > At minimum that would require > * not generating extra copies of the function Even if someone uses such a compiler, maybe the extra copies are not too bad. These are small functions. If not inlined, the compiled code size on x86-32 islist_head 48 byteslist_tail 48 byteslist_length 48 bytesMemoryContextSwitchTo 32 bytes Total 176 bytes In src/backend there are 478 .c files that #include postgres.h, so the potential bloat is about 82 KB (= 478 * 176). This would also occur anytime the user specifies compiler options that suppress inlining, such as for a debug build; but then the executable size doesn't matter usually. > * not whining about unreferenced static functions I could update the patch so that "configure" will test for this. (BTW, MSVC is a whiner, but clams up if __forceinline is used.) > How many compilers have you tested this patch against? Only a small number. By submitting it to pgsql-hackers, I hope that it can be exposed to broader coverage. > Which ones does it actually offer any benefit for? MSVC is one. I hope eventually it will be found that all compilers of interest do a good enough job with static inline functions so that we can use a lot more of them. For example, I'd like to expunge the abominable heap_getattr() and fastgetattr() macros in access/htup.h and replace them with an inline function. Such improvements are less easily justifiable if they are limited to gcc. Regards, ... kurt
Bruce Momjian wrote: > Peter Eisentraut wrote: >> On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote: >>> I thought one problem was that inline is a suggestion that the compiler >>> can ignore, while macros have to be implemented as specified. >> Sure, but one could argue that a compiler that doesn't support inline >> usefully is probably not the sort of compiler that you use for compiling >> performance-relevant software anyway. We can support such systems in a >> degraded way for historical value and evaluation purposes as long as >> it's pretty much free, like we support systems without working int8. > > The issue is that many compilers will take "inline" as a suggestion and > decide if it is worth-while to inline it --- I don't think it is inlined > unconditionally by any modern compilers. > > Right now we think we are better at deciding what should be inlined than > the compiler --- of course, we might be wrong, and it would be good to > performance test this. Just to clarify, this patch doesn't add new inline functions or convert any macros to inline functions. At present, PostgreSQL header files define four functions as inline for gcc only. This patch merely opens up those existing inline functions to be used with any compiler that accepts them, not just gcc. If this goes well, it may be a preparatory step toward future adoption of more inline functions and fewer macros. Regards, ... kurt
Hi, Attached is a revised patch, offered for the 2010-01 commitfest. It's also available in my git repository in the "submitted" branch: http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted In this version, the "configure" script tests whether a static inline function can be defined without incurring a warning when not referenced. If successful, the preprocessor symbol PG_INLINE is defined in pg_config.h to the appropriate keyword: inline, __inline, __inline__, or __forceinline. Otherwise PG_INLINE remains undefined. palloc.h and pg_list.h condition their inline function definitions on PG_INLINE instead of the gcc-specific __GNUC__. Thus the functions can be inlined on more platforms, not only gcc. Ordinary out-of-line calls are still used if the compiler doesn't recognize inline functions, or spews warnings when static inline functions are defined but not referenced. Regards, ... kurt
Attachment
On 12/15/09, Kurt Harriman <harriman@acm.org> wrote: > Attached is a revised patch, offered for the 2010-01 commitfest. > It's also available in my git repository in the "submitted" branch: > > http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted > > In this version, the "configure" script tests whether a static > inline function can be defined without incurring a warning when > not referenced. If successful, the preprocessor symbol PG_INLINE > is defined in pg_config.h to the appropriate keyword: inline, > __inline, __inline__, or __forceinline. Otherwise PG_INLINE > remains undefined. > > palloc.h and pg_list.h condition their inline function > definitions on PG_INLINE instead of the gcc-specific __GNUC__. > Thus the functions can be inlined on more platforms, not only > gcc. > > Ordinary out-of-line calls are still used if the compiler doesn't > recognize inline functions, or spews warnings when static inline > functions are defined but not referenced. -1. The PG_INLINE is ugly. In actual C code we should see only "inline" keyword, no XX_INLINE, __inline, __inline__, etc. It's up to configure to make sure "inline" is something that C compiler accepts or simply defined to empty string otherwise. I assume you are playing with force-inline to avoid warnings on some compiler? Do you a actual compiler that behaves like that? Unless it is some popular compiler (as "in actual use") it is premature complexity. Please remove that. We may want to have force-inline in the future, when we start converting some more complex macros to inline functions, but then it should cover all main compilers, and current patch does not try to do it. So my suggestions: 1. Make sure "inline" is defined, empty or to something that works. (plain AC_C_INLINE seems to do that) 2. Convert current __inline, __inline__ sites to "inline" and 3. Remove #ifdefs and duplicate macros, keeping only inline funcs. There does not seem to be any holding counter-arguments why we should not do that, so lets go ahead? -- marko
On 12/15/2009 4:05 AM, Marko Kreen wrote: > On 12/15/09, Kurt Harriman<harriman@acm.org> wrote: >> Attached is a revised patch, offered for the 2010-01 commitfest. >> It's also available in my git repository in the "submitted" branch: >> >> http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted >> > -1. The PG_INLINE is ugly. > > In actual C code we should see only "inline" keyword, no XX_INLINE, > __inline, __inline__, etc. It's up to configure to make sure "inline" > is something that C compiler accepts or simply defined to empty string > otherwise. > > I assume you are playing with force-inline to avoid warnings on some > compiler? Do you a actual compiler that behaves like that? Unless > it is some popular compiler (as "in actual use") it is premature > complexity. Please remove that. > > We may want to have force-inline in the future, when we start converting > some more complex macros to inline functions, but then it should cover > all main compilers, and current patch does not try to do it. > > So my suggestions: > > 1. Make sure "inline" is defined, empty or to something that works. > (plain AC_C_INLINE seems to do that) > 2. Convert current __inline, __inline__ sites to "inline" > > and > > 3. Remove #ifdefs and duplicate macros, keeping only inline funcs. > > There does not seem to be any holding counter-arguments why we should > not do that, so lets go ahead? Hi Marko, Thanks for your comments. Although I agree that the nicest code would be as you suggest, and that would work fine with gcc, I hesitate to dismiss so quickly the counter-arguments: i) That would reduce the number of platforms on which thebackend compiles cleanly with no warnings. Microsoft Cplatformswould be among those affected. ii) Your item #3 would unnecessarily increase the size ofthe postgres executable on platforms where no inline keywordis recognizedand unused static functions aren't optimizedaway. The increase would be about 82 KB if this were tooccur on anx86-32 platform, for example. Maybe nobody stilluses such an old compiler, or if they do, maybe the increasedexecutablesize would be small enough to ignore; but I don'tknow this. Users who would be affected are not necessarilyreadersof the pgsql-hackers list. This patch is meant to broaden the number of platforms benefiting from inlining. Also it is hoped to facilitate greater use of inline functions in the future, in preference to macros. I don't want to disadvantage the older platforms, unless they are unambiguously not important anymore. > -1. The PG_INLINE is ugly. Yes, but not quite as ugly as the existing code. It is more portable, eliminating hard coded gcc dependencies in several places. > 1. Make sure "inline" is defined, empty or to something that works.> (plain AC_C_INLINE seems to do that) Yes, it is already that way in the existing code. If the compiler doesn't accept plain "inline", then the "configure" script sets up pg_config.h to #define inline as a preprocessor symbol that maps to __inline, __inline__, or empty. As you say, plain AC_C_INLINE takes care of that. My patch doesn't affect this. > In actual C code we should see only "inline" keyword, no XX_INLINE,> __inline, __inline__, etc. It's up to configure tomake sure "inline"> is something that C compiler accepts or simply defined to empty string> otherwise. Yes, that is already how we define inline functions in .c files such as executor/nodeSetOp.c, optimizer/path/joinpath.c, and storage/lmgr/lock.c. It seems to work fine there. However, there is a difference between .c files and .h files. A static inline definition in a .c file is certain to be referenced, and is never instantiated more than once even if not inlined. Therefore in .c files we need not be concerned about the two issues mentioned above: superfluous "defined but not used" warnings, and unnecessary bloating of the executable. On the other hand, a static inline definition in a .h file is liable to be #included into (perhaps hundreds of) compilations where it is not referenced. Some compilers conscientiously display a warning every time this happens. Some emit out-of-line code every time they see the definition, even if it is not called (likeliest when inline is not supported at all and is #defined to empty). To avoid these problems, historically the PostgreSQL developers have minimized the use of inline functions in header files, and instead used macros. In the two header files where macros were considered too ugly, inline functions were conditionally compiled for gcc only. At that time, __inline__ was a nonstandard gcc extension. Nowadays, inline functions are part of standard C99 and C++. For static inline functions, the C99 and C++ standard behavior is basically the same as the old gcc extension. So by now, most compilers support this somehow, and it makes sense to remove the gcc dependency. > I assume you are playing with force-inline to avoid warnings on some> compiler? Do you a actual compiler that behaveslike that? Yes, Microsoft compilers warn about unreferenced static functions, but keep quiet when the function is defined with __forceinline. Inline functions defined in header files are typically small enough that compilers will always obey the inline directive, so it doesn't matter whether plain inline or __forceinline is used, except for the warnings. This is certainly the case for the four inline functions which exist at present in palloc.h and pg_list.h. So far in my experience outside PostgreSQL, compilers almost always inline everything that I have defined as inline. I've never had occasion to use __forceinline with a modern compiler, except now for the MSVC warnings. > Unless it is some popular compiler (as "in actual use") it is> premature complexity. Please remove that. Microsoft's compilers are in actual use, and some might even say they are popular. For example, James Mansion posted to this effect on 2 December. > We may want to have force-inline in the future, when we start converting> some more complex macros to inline functions,but then it should cover> all main compilers, and current patch does not try to do it. At present I do not know of any reason why we should want __forceinline, except here for its side-effect of silencing MSVC's warnings about static functions that are defined but not used. Instead of using __forceinline, it would be possible to use CFLAGS or a #pragma to globally suppress the unreferenced symbol warnings. However, the warning codes seem less portable because they may be specific to one compiler or change between releases. Also, useful warnings might be hidden along with the superfluous ones. Regards, ... kurt
On 12/15/09, Kurt Harriman <harriman@acm.org> wrote: > On 12/15/2009 4:05 AM, Marko Kreen wrote: > > Unless it is some popular compiler (as "in actual use") it is > > premature complexity. Please remove that. > > Microsoft's compilers are in actual use, and some might even > say they are popular. For example, James Mansion posted to > this effect on 2 December. Erm, AFAIK he simply misunderstood my "(gcc)" comment. Do you have actual proof that MSVC launches warnings on unused "static inline" functions? Not "static", but "static inline". If yes, indeed we need to fix it. MSVC is broken then, but it does not matter as we need to work well on it. We can fix it with either force-inline, or equivalent with gcc's __attribute__((unused)). If no, then we don't need to fix it. Adding complexity based on some email miscommunication seems wrong to me... -- marko
On 12/15/2009 1:31 PM, Marko Kreen wrote: > Do you have actual proof that MSVC launches warnings on unused > "static inline" functions? Not "static", but "static inline". Yes, I tried it, and that's why I did it this way. > If yes, indeed we need to fix it. MSVC is broken then, but it does > not matter as we need to work well on it. We can fix it with either > force-inline, or equivalent with gcc's __attribute__((unused)). Microsoft doesn't support the gcc __attribute__ syntax, AFAIK. They have a few other gewgaws, like __declspec__, but I didn't find one that helps with this problem. Regards, ... kurt
On 12/15/09, Kurt Harriman <harriman@acm.org> wrote: > On 12/15/2009 1:31 PM, Marko Kreen wrote: > > > Do you have actual proof that MSVC launches warnings on unused > > "static inline" functions? Not "static", but "static inline". > > Yes, I tried it, and that's why I did it this way. Oh. Ok then. Force-inline seems better fix as we may want to use it for other reasons too (converting big macros). So it seems like a good moment to solve it for gcc too. Your worry ii) can be ignored, managing to compile on such compilers is already overachievement. > > If yes, indeed we need to fix it. MSVC is broken then, but it does > > not matter as we need to work well on it. We can fix it with either > > force-inline, or equivalent with gcc's __attribute__((unused)). > > > > Microsoft doesn't support the gcc __attribute__ syntax, AFAIK. > > They have a few other gewgaws, like __declspec__, but I didn't > find one that helps with this problem. The question is now what should we put into configure and what into headers. Simplest would be to have plain AC_C_INLINE in configure and then in header (c.h?): #ifdef _MSC_VER #undef inline #define inline __forceinline #else #ifdef __GNUC__ #undef inline #define inline inline __attribute__((always_inline)) #endif #endif (Not compile tested :) Or should we put that logic also into configure, so that the config.h already contains proper 'inline'? Although fitting it into win32 buildsystem seems to be bit more complex in that case... -- marko
On 12/15/2009 2:09 PM, Marko Kreen wrote: > > Oh. Ok then. Force-inline seems better fix as we may want to use > it for other reasons too (converting big macros). > > So it seems like a good moment to solve it for gcc too. Is ordinary inlining not sufficient? If deluxe inlining is something that might be wanted in the future, but isn't needed right now, perhaps we should wait until then. > Your worry ii) can be ignored, managing to compile on such > compilers is already overachievement. I think so too. With your opinion added to mine, do we constitute a consensus of the pg community? Someone might object that a sample of two individuals is insufficiently representative of the whole, but away with the pedants: let us not quibble over trifles. > The question is now what should we put into configure and what into > headers. PostgreSQL seems mostly to follow the GNU > Simplest would be to have plain AC_C_INLINE in configure > and then in header (c.h?): > > #ifdef _MSC_VER > #undef inline > #define inline __forceinline > #else > #ifdef __GNUC__ > #undef inline > #define inline inline __attribute__((always_inline)) > #endif > #endif > > (Not compile tested :) > > Or should we put that logic also into configure, > so that the config.h already contains proper 'inline'? > > Although fitting it into win32 buildsystem seems to be bit more > complex in that case...
[Please ignore the previous incomplete version of this reply, which I sent by mistake. Sorry for the list noise.] On 12/15/2009 2:09 PM, Marko Kreen wrote:>> Oh. Ok then. Force-inline seems better fix as we may want to use> it for otherreasons too (converting big macros).>> So it seems like a good moment to solve it for gcc too. Is ordinary inlining not sufficient? If deluxe inlining is something that might be wanted in the future, but isn't needed right now, perhaps we should wait until then. > Your worry ii) can be ignored, managing to compile on such> compilers is already overachievement. I think so too. With your opinion added to mine, do we constitute a consensus of the pg community? Someone might object that a sample of two individuals is insufficiently representative of the whole, but away with the pedants: let us not quibble over trifles. > The question is now what should we put into configure and what into> headers. PostgreSQL seems mostly to follow the GNU tradition of using autoconf rather than thickets of #ifdefs to discover platform characteristics such as supported features, variant spellings of keywords and identifiers, and the like. This often makes it easier to support new platforms, assuming the autoconf mechanism is understood. Autoconf facilitates testing directly for the needed features: generally a better approach than hard-coding knowledge of the peculiarities of particular compilers, compiler release levels, instruction set architectures, etc. For example, instead of writing #ifdefs to decide, "if this is gcc, and the version is high enough, then __funcname__ should work", it's better to let autoconf actually try to compile a program using __funcname__. That way PostgreSQL can keep up with the evolution of improved and new compilers without continually updating an #ifdef-encoded knowledge base of compiler capabilities. > Simplest would be to have plain AC_C_INLINE in configure> and then in header (c.h?):>> #ifdef _MSC_VER> #undef inline>#define inline __forceinline> #else> #ifdef __GNUC__> #undef inline> #define inline inline __attribute__((always_inline))>#endif> #endif>> (Not compile tested :) This would force every inline. Why do we want that? For gcc, I think the __attribute__ has to come after the function's parameter list, rather than before the return type. > Or should we put that logic also into configure,> so that the config.h already contains proper 'inline'? That's how it works at present for plain (not forced) inline. Regards, ... kurt
On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman <harriman@acm.org> wrote: >> Your worry ii) can be ignored, managing to compile on such >> compilers is already overachievement. > > I think so too. With your opinion added to mine, do we constitute a > consensus of the pg community? Someone might object that a sample of > two individuals is insufficiently representative of the whole, but > away with the pedants: let us not quibble over trifles. I haven't completely followed this thread, but I think there has been some discussion of making changes to inline that would cause regressions for people using old, crappy compilers, and I think we should avoid doing that unless there is some compelling benefit. I'm not sure what that benefit would be - I don't think "cleaner code" is enough. ...Robert
On 12/15/2009 9:42 PM, Robert Haas wrote: > On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman<harriman@acm.org> wrote: >>> Your worry ii) can be ignored, managing to compile on such >>> compilers is already overachievement. >> >> I think so too. With your opinion added to mine, do we constitute a >> consensus of the pg community? Someone might object that a sample of >> two individuals is insufficiently representative of the whole, but >> away with the pedants: let us not quibble over trifles. > > I haven't completely followed this thread, but I think there has been > some discussion of making changes to inline that would cause > regressions for people using old, crappy compilers, and I think we > should avoid doing that unless there is some compelling benefit. I'm > not sure what that benefit would be - I don't think "cleaner code" is > enough. > > ...Robert Hmm, this sample of 3 has a lot of variance now. When I referred to "consensus", perhaps that was optimism? In fact I cannot disagree with Robert's comment. Regards, ... kurt
On 12/16/09, Kurt Harriman <harriman@acm.org> wrote: > [Please ignore the previous incomplete version of this reply, which I > sent by mistake. Sorry for the list noise.] > > On 12/15/2009 2:09 PM, Marko Kreen wrote: > > > > Oh. Ok then. Force-inline seems better fix as we may want to use > > it for other reasons too (converting big macros). > > > > So it seems like a good moment to solve it for gcc too. > > Is ordinary inlining not sufficient? > > If deluxe inlining is something that might be wanted in the > future, but isn't needed right now, perhaps we should wait > until then. It allows the effects the __forceinline has on MSVC also observe on GCC. It should not hurt anything, unless we go overboard with inline usage. But yeah, it can be omitted. I was just wondering on code structure, to make it possible to turn forceinline for gcc too. > > Your worry ii) can be ignored, managing to compile on such > > compilers is already overachievement. > > I think so too. With your opinion added to mine, do we constitute a > consensus of the pg community? Someone might object that a sample of > two individuals is insufficiently representative of the whole, but > away with the pedants: let us not quibble over trifles. Note that we are not voting here, we are on technical discussion list. Interested people can voice counter-arguments, if we can overturn them, we can proceed... > > The question is now what should we put into configure and what into > > headers. > > PostgreSQL seems mostly to follow the GNU tradition of using > autoconf rather than thickets of #ifdefs to discover platform > characteristics such as supported features, variant spellings of > keywords and identifiers, and the like. This often makes it easier > to support new platforms, assuming the autoconf mechanism is > understood. > > Autoconf facilitates testing directly for the needed features: > generally a better approach than hard-coding knowledge of the > peculiarities of particular compilers, compiler release levels, > instruction set architectures, etc. For example, instead of > writing #ifdefs to decide, "if this is gcc, and the version is > high enough, then __funcname__ should work", it's better to let > autoconf actually try to compile a program using __funcname__. > That way PostgreSQL can keep up with the evolution of improved > and new compilers without continually updating an #ifdef-encoded > knowledge base of compiler capabilities. Well, yeah, except on one platform we are mostly bypassing the autoconf logic and have lot of duplicate logic. But I'm not opposed putting it into configure, assuming the final keyword is still called "inline". > > Simplest would be to have plain AC_C_INLINE in configure > > and then in header (c.h?): > > > > #ifdef _MSC_VER > > #undef inline > > #define inline __forceinline > > #else > > #ifdef __GNUC__ > > #undef inline > > #define inline inline __attribute__((always_inline)) > > #endif > > #endif > > > > (Not compile tested :) > > This would force every inline. Why do we want that? The compiler is allowed to uninline a plain "inline" function if it feels like it. The gcc uses guesstimate instruction count (before inlining) to decide on that issue. ATM it's not a problem, but eg. if we want to turn more complex macros into inlines (heap_getattr()), it may go over limit and get uninlined. > For gcc, I think the __attribute__ has to come after the function's > parameter list, rather than before the return type. No. -- marko
On 12/16/09, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman <harriman@acm.org> wrote: > >> Your worry ii) can be ignored, managing to compile on such > >> compilers is already overachievement. > > > > I think so too. With your opinion added to mine, do we constitute a > > consensus of the pg community? Someone might object that a sample of > > two individuals is insufficiently representative of the whole, but > > away with the pedants: let us not quibble over trifles. > > > I haven't completely followed this thread, but I think there has been > some discussion of making changes to inline that would cause > regressions for people using old, crappy compilers, and I think we > should avoid doing that unless there is some compelling benefit. I'm > not sure what that benefit would be - I don't think "cleaner code" is > enough. Seems you have not followed the thread... Hypothetical old, crappy compilers would still work, only AC_C_INLINE would turn "static inline" into plain "static", so hypothetically they would get some warnings about unused functions. As this is all hypothetical, I don't see why that should stop us cleaning our code? -- marko
Marko Kreen <markokr@gmail.com> writes:
> On 12/16/09, Kurt Harriman <harriman@acm.org> wrote:
>> For gcc, I think the __attribute__ has to come after the function's
>> parameter list, rather than before the return type.
> No.
[ squint... ] That's nowhere documented that I can find: all the
examples in the gcc docs show __attribute__ after the parameters.
It does seem to work, but should we rely on it?
The bigger problem though is that not all versions of gcc understand
always_inline:
$ gcc -Wall check.c
check.c:3: warning: `always_inline' attribute directive ignored
which I think is sufficient reason to put an end to this sub-thread.
We have no particular need for force-inline semantics anyway, as
long as the compiler behaves reasonably for unreferenced inlines,
which gcc always has.
regards, tom lane
On Wed, Dec 16, 2009 at 9:30 AM, Marko Kreen <markokr@gmail.com> wrote: > On 12/16/09, Robert Haas <robertmhaas@gmail.com> wrote: >> On Tue, Dec 15, 2009 at 10:34 PM, Kurt Harriman <harriman@acm.org> wrote: >> >> Your worry ii) can be ignored, managing to compile on such >> >> compilers is already overachievement. >> > >> > I think so too. With your opinion added to mine, do we constitute a >> > consensus of the pg community? Someone might object that a sample of >> > two individuals is insufficiently representative of the whole, but >> > away with the pedants: let us not quibble over trifles. >> >> >> I haven't completely followed this thread, but I think there has been >> some discussion of making changes to inline that would cause >> regressions for people using old, crappy compilers, and I think we >> should avoid doing that unless there is some compelling benefit. I'm >> not sure what that benefit would be - I don't think "cleaner code" is >> enough. > > Seems you have not followed the thread... > > Hypothetical old, crappy compilers would still work, only AC_C_INLINE > would turn "static inline" into plain "static", so hypothetically > they would get some warnings about unused functions. > > As this is all hypothetical, I don't see why that should stop us > cleaning our code? I don't think that hypothetical is the right word. There is no question that such compilers exist. What seems to me to be relevant is whether anyone is still using them. Maybe with sufficient research you could demonstrate that there are no platforms we support where these are still in common use - e.g. the oldest version of AIX compiler that has this problem is version X, but PostgreSQL is only supported on versions Y and higher, where Y>X. But I don't see that any attempt has been made to do that research, or at least I haven't seen any attempt to go through our supported platforms and discuss the situation for each one. I'm not really sure there's enough benefit in this project to warrant that effort, but that's up to you to decide. However, I am pretty confident that there is going to be opposition to degrading performance or causing lots of compiler warnings on older platforms that are still supported. In fact, such opposition has already been registered on this thread and you can find it here: http://archives.postgresql.org/pgsql-hackers/2009-12/msg00239.php Our standard for accepting patches is that (1) they have a demonstrated benefit and (2) they don't break anything. Leaving aside the question of whether this patch makes anything worse, it would really improve the case for this patch if someone could enumerate the supported platforms and compilers where it makes things better. ...Robert
Marko Kreen <markokr@gmail.com> writes:
> Hypothetical old, crappy compilers would still work, only AC_C_INLINE
> would turn "static inline" into plain "static", so hypothetically
> they would get some warnings about unused functions.
> As this is all hypothetical, I don't see why that should stop us
> cleaning our code?
There's nothing "hypothetical" about it --- I still regularly check
that the code builds on an old HP compiler that doesn't have inline.
I remind you that the project policy is to not require any compiler
features not found in C89. If you can exploit inline on more compilers
than now, fine, but assuming that everything has got it is not OK.
regards, tom lane
On 12/16/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > On 12/16/09, Kurt Harriman <harriman@acm.org> wrote: > > >> For gcc, I think the __attribute__ has to come after the function's > >> parameter list, rather than before the return type. > > > No. > > > [ squint... ] That's nowhere documented that I can find: all the > examples in the gcc docs show __attribute__ after the parameters. > It does seem to work, but should we rely on it? Heh. At least in 3.0, 3.1 and 4.2 docs there is: __attribute__((noreturn)) void d0 (void), __attribute__((format(printf, 1, 2))) d1 (const char *, ...), d2 (void); describing that _att before decl list applies to all declarations, but _att after decl applies to only that declaration. That sort of explains also why all examples have _att afterwards. http://gcc.gnu.org/onlinedocs/gcc-3.1.1/gcc/Attribute-Syntax.html But I dare not to pick out sentences from Function-Attributes.html that describe that... > The bigger problem though is that not all versions of gcc understand > always_inline: > > $ gcc -Wall check.c > check.c:3: warning: `always_inline' attribute directive ignored Oh, another argument against logic in headers. > which I think is sufficient reason to put an end to this sub-thread. > We have no particular need for force-inline semantics anyway, as > long as the compiler behaves reasonably for unreferenced inlines, > which gcc always has. Ok. -- marko
On 12/16/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > Hypothetical old, crappy compilers would still work, only AC_C_INLINE > > would turn "static inline" into plain "static", so hypothetically > > they would get some warnings about unused functions. > > > As this is all hypothetical, I don't see why that should stop us > > cleaning our code? > > There's nothing "hypothetical" about it --- I still regularly check > that the code builds on an old HP compiler that doesn't have inline. Ok, good. Thus far only argument was "historically they have existed", which does not sound good enough worry about them. If somebody is actually testing and caring abouth such compilers, they need to be taken more seriously. > I remind you that the project policy is to not require any compiler > features not found in C89. If you can exploit inline on more compilers > than now, fine, but assuming that everything has got it is not OK. Note - my advanced proposal (instead duplicate macros, let 'static inline' functions fall back to being plain 'static') would still support non-inline compilers, but with potentially small performance hit. So the plain-C89 compilers would be downgraded to "second-class" targets, not worth getting max performance out of them. Is this OK? -- marko
On 12/16/2009 7:10 AM, Tom Lane wrote: > the project policy is to not require any compiler features not found> in C89. Is there somewhere a compendium of such policies which fledgling hackers should consult to avoid embarrassment? Regards, ... kurt
Marko Kreen <markokr@gmail.com> writes:
> So the plain-C89 compilers would be downgraded to "second-class"
> targets, not worth getting max performance out of them.
Hm? Failing to inline is already a performance hit, which is why
Kurt got interested in this in the first place.
I think you're way overthinking this. Where we started was just
a proposal to try to expand the set of inline-ing compilers beyond
"gcc only". I don't see why we need to do anything but that. The
code is fine as-is except for the control #ifdefs.
regards, tom lane
On ons, 2009-12-16 at 07:44 -0800, Kurt Harriman wrote: > On 12/16/2009 7:10 AM, Tom Lane wrote: > > the project policy is to not require any compiler features not found > > in C89. > > Is there somewhere a compendium of such policies which > fledgling hackers should consult to avoid embarrassment? The installation instructions say that you need a C89 compiler.
On Wed, Dec 16, 2009 at 10:44 AM, Kurt Harriman <harriman@acm.org> wrote: > On 12/16/2009 7:10 AM, Tom Lane wrote: >> >> the project policy is to not require any compiler features not found > >> in C89. > > Is there somewhere a compendium of such policies which > fledgling hackers should consult to avoid embarrassment? Well the best way to avoid embarrassment is to just not be embarrassed. I don't think anyone will think ill of you for suggesting something that turns out not to be where the project wants to go, as long as you're not too dogmatic about it. This particular point is mentioned in the documentation: http://www.postgresql.org/docs/8.4/static/install-requirements.html The rest of chapter 15 is worth a read, too. I do think your basic point is well-taken, though. There are a lot of details about how we do things that I have had to learn through osmosis, and maybe if I can figure out what they all are, I'll try to write them down somewhere, if someone else hasn't done it already. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 16, 2009 at 10:44 AM, Kurt Harriman <harriman@acm.org> wrote:
>> Is there somewhere a compendium of such policies which
>> fledgling hackers should consult to avoid embarrassment?
> I do think your basic point is well-taken, though. There are a lot of
> details about how we do things that I have had to learn through
> osmosis, and maybe if I can figure out what they all are, I'll try to
> write them down somewhere, if someone else hasn't done it already.
Yeah, this is something that is difficult for the senior hackers to
undertake because we've forgotten what it was we had to learn the hard
way ;-). Maybe some of the newer project members could collaborate
on scribbling down such stuff (wiki page?).
regards, tom lane
Tom Lane escribió: > I think you're way overthinking this. Where we started was just > a proposal to try to expand the set of inline-ing compilers beyond > "gcc only". I don't see why we need to do anything but that. The > code is fine as-is except for the control #ifdefs. IIRC Kurt was also on about getting rid of some ugly macros that could instead be coded as inline functions (fastgetattr for example) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Marko Kreen <markokr@gmail.com> writes: > On 12/15/09, Kurt Harriman <harriman@acm.org> wrote: >> Attached is a revised patch, offered for the 2010-01 commitfest. >> It's also available in my git repository in the "submitted" branch: >> >> http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted > -1. The PG_INLINE is ugly. FWIW, I think the patch is largely OK, except for the autoconf hackery which I'm not the best-qualified person to opine on. I would only suggest that the cleanest coding would be #ifdef USE_INLINE static inline foo(...) ... #else ... non-inline definition of foo #endif ie, go ahead and rely on autoconf's definition (if any) of "inline" and add a policy symbol USE_INLINE to determine whether to use it. The proposed PG_INLINE coding conflates the symbol needed in the code with the policy choice. Another possibility would be to call the policy symbol HAVE_INLINE, but that (a) risks collision with a name defined by autoconf built-in macros, and (b) looks like it merely indicates whether the compiler *has* inline, not that we have made a choice about how to use it. regards, tom lane
On 12/16/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > > So the plain-C89 compilers would be downgraded to "second-class" > > targets, not worth getting max performance out of them. > > > Hm? Failing to inline is already a performance hit, which is why > Kurt got interested in this in the first place. > > I think you're way overthinking this. Where we started was just > a proposal to try to expand the set of inline-ing compilers beyond > "gcc only". I don't see why we need to do anything but that. The > code is fine as-is except for the control #ifdefs. My proposal is basically about allowing more widespread use of "static inline". That is - "static inline" does not need to be paired with equivalent macro. But if C89 compilers are still project's primary target, then this cannot be allowed. -- marko
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane escribi�:
>> I think you're way overthinking this. Where we started was just
>> a proposal to try to expand the set of inline-ing compilers beyond
>> "gcc only". I don't see why we need to do anything but that. The
>> code is fine as-is except for the control #ifdefs.
> IIRC Kurt was also on about getting rid of some ugly macros that could
> instead be coded as inline functions (fastgetattr for example)
I'd just bounce that as useless activity. If they are macros now,
and work, the only possible effects of changing them are negative.
regards, tom lane
Kurt Harriman wrote: > On 12/16/2009 7:10 AM, Tom Lane wrote: >> the project policy is to not require any compiler features not found > > in C89. > > Is there somewhere a compendium of such policies which > fledgling hackers should consult to avoid embarrassment? The list of suggestions at http://wiki.postgresql.org/wiki/Developer_FAQ and http://wiki.postgresql.org/wiki/Submitting_a_Patch are what we've got now. There's a number of us who try to record any interesting discussion bits like this one into one of those, to shorten the future learning curve for others. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg@2ndQuadrant.com www.2ndQuadrant.com
On ons, 2009-12-16 at 10:49 -0500, Tom Lane wrote: > Marko Kreen <markokr@gmail.com> writes: > > So the plain-C89 compilers would be downgraded to "second-class" > > targets, not worth getting max performance out of them. > > Hm? Failing to inline is already a performance hit, which is why > Kurt got interested in this in the first place. > > I think you're way overthinking this. Where we started was just > a proposal to try to expand the set of inline-ing compilers beyond > "gcc only". I don't see why we need to do anything but that. The > code is fine as-is except for the control #ifdefs. I think the ifdefs should just be HAVE_INLINE && !MSVC, right?
On ons, 2009-12-16 at 10:49 -0500, Tom Lane wrote: > I think you're way overthinking this. Where we started was just > a proposal to try to expand the set of inline-ing compilers beyond > "gcc only". I don't see why we need to do anything but that. The > code is fine as-is except for the control #ifdefs. I have found an Autoconf macro that checks whether the compiler properly supports C99 inline semantics. This would allow us to replace the __GNUC__ conditional with HAVE_C99_INLINE, in this case. Interestingly, however, this check results in GCC <=4.2 *not* supporting C99 inline, apparently because it produces redundant copies of static inline functions. But GCC 4.2 is currently Debian stable, for example, so de-supporting that is probably not yet an option. So, I'm not sure if this would actually solve anyone's problem, but it does call into question that exact semantics that we are looking for. Maybe we just replace __GNUC__ by __GNUC__ || __SOMETHING_ELSE_CC__. Patch attached for entertainment.
Attachment
Peter Eisentraut <peter_e@gmx.net> writes:
> I have found an Autoconf macro that checks whether the compiler properly
> supports C99 inline semantics. This would allow us to replace the
> __GNUC__ conditional with HAVE_C99_INLINE, in this case. Interestingly,
> however, this check results in GCC <=4.2 *not* supporting C99 inline,
> apparently because it produces redundant copies of static inline
> functions. But GCC 4.2 is currently Debian stable, for example, so
> de-supporting that is probably not yet an option.
Some of us are using much older gcc's than that, too ;-). But actually
I think this test is 100% bogus anyhow. What it appears to rely on is
the compiler failing to do semantic error testing on an inline function
that it doesn't need to instantiate. That's a behavior that I'm pretty
sure is gcc-specific rather than required by C99, and in any case has
very little to do with our requirements.
regards, tom lane
On sön, 2009-12-06 at 02:21 -0800, Kurt Harriman wrote: > > Which ones does it actually offer any benefit for? > > MSVC is one. I seem to recall that somewhere else it was said that MSVC produces warnings on unused static inline functions. Am I mistaken? Also, if this is mainly for the benefit of MSVC, we don't really need a configure check, do we?
On 1/18/2010 1:20 PM, Peter Eisentraut wrote: > I seem to recall that somewhere else it was said that MSVC produces > warnings on unused static inline functions. Am I mistaken? MSVC does warn about unused static inline functions. The warning is prevented by using __forceinline instead of __inline. > Also, if this is mainly for the benefit of MSVC, we don't really need a > configure check, do we? A configure check seems worthwhile regardless of MSVC. No doubt other compilers, yet unknown, will be discovered to spew alarm upon unreferenced static inline functions. To address the difficulty of testing innumerable present and future platforms, it seems there are three alternatives: a) Wait for complaints, then craft compiler-specific patches and backpatches. b) Keep the existing compiler-specific logic to enable inlining for gcc alone. Craft additional compiler-specificlogic for each additional trusted and sufficiently important compiler. c) Use configure to automate the testing of each build environment in situ. The third alternative adapts to new or little-known platforms with little or no manual intervention. It factors platform dependencies out of the code, centralizing them into the build system. It is true that configure doesn't need to test for MSVC's __forceinline keyword. I included that mainly as a placeholder for the benefit of future hackers: likely someone will discover a need for a special keyword to suppress another compiler's warnings. With __forceinine as an example, and the looping logic already in place, it's easy to add an alternative keyword to the list without having to become an autoconf expert. It doesn't make configure slower, since gcc succeeds on the first iteration without trying the __forceinline alternative. Regards, ... kurt
Kurt Harriman <harriman@acm.org> writes:
> c) Use configure to automate the testing of each build environment
> in situ.
> The third alternative adapts to new or little-known platforms
> with little or no manual intervention.
This argument is bogus unless you can demonstrate a working configure
probe for the property in question. The question about this patch,
from day one, has been whether we have a working configure test.
> It is true that configure doesn't need to test for MSVC's
> __forceinline keyword. I included that mainly as a placeholder
> for the benefit of future hackers: likely someone will
> discover a need for a special keyword to suppress another
> compiler's warnings.
I think including MSVC in the set of compilers targeted by a configure
test is just a waste of code. It's more likely to confuse people than
help them.
regards, tom lane
On 1/18/2010 4:42 PM, Tom Lane wrote:> Kurt Harriman<harriman@acm.org> writes:>> c) Use configure to automate the
testingof each build environment>> in situ.>>> The third alternative adapts to new or little-known platforms>>
withlittle or no manual intervention.>> This argument is bogus unless you can demonstrate a working configure> probe
forthe property in question. The question about this patch,> from day one, has been whether we have a working
configuretest.
It does work to detect almost exactly the desired property:
the absence of a compiler or linker warning when a static
inline function is defined but not referenced.
"Almost" exactly, because rather than absence of errors or
warnings, the exact condition tested is: successful exit
code and nothing whatever written to stdout or stderr.
(That is a built-in feature of autoconf.)
Its success is easily demonstrated on any platform by editing the
code within AC_LINK_PROGRAM to induce a compiler or linker warning.
>> It is true that configure doesn't need to test for MSVC's>> __forceinline keyword. I included that mainly as a
placeholder>>for the benefit of future hackers: likely someone will>> discover a need for a special keyword to
suppressanother>> compiler's warnings.>> I think including MSVC in the set of compilers targeted by a configure> test
isjust a waste of code. It's more likely to confuse people than> help them.
There's a loop for trying a series of compiler-specific
formulas. The list contains two items: (1) "inline" or
equivalent as determined by AC_C_INLINE; (2) __forceinline.
In a 2-item list, it is easy to see the syntax for adding a
third item: in this case, the language is 'sh' and the list
is space-separated. The code is:
for pgac_kw in "$ac_cv_c_inline" "__forceinline" ; do AC_LINK_IFELSE([AC_LANG_PROGRAM([static $pgac_kw int fun
(){return 0;}],[])], [pgac_cv_c_hushinline=$pgac_kw]) test "$pgac_cv_c_hushinline" != no &&
break done
MSVC __forceinline could be dropped from the list:
for pgac_kw in "$ac_cv_c_inline" ; do AC_LINK_IFELSE([AC_LANG_PROGRAM([static $pgac_kw int fun () {return
0;}],[])], [pgac_cv_c_hushinline=$pgac_kw]) test "$pgac_cv_c_hushinline" != no && break
done
Since the list would then have only one element, the
whole loop could be dropped.
AC_LINK_IFELSE([AC_LANG_PROGRAM([static $pgac_kw int fun () {return 0;}],[])],
[pgac_cv_c_hushinline=$pgac_kw])
Someone could reinstate the loop if it is discovered that MSVC
is not the only compiler which needs something special to
silence its unused-function warning.
Regards,
... kurt
On mån, 2010-01-18 at 16:34 -0800, Kurt Harriman wrote: > MSVC does warn about unused static inline functions. The warning > is prevented by using __forceinline instead of __inline. Hmm, but forceinline is not the same as inline. Are we confident that forcing inlining is not going to lead to disadvantages? Has this been measured? Is there not a setting to disable this particular warning. I read that MSVC has various ways to set that sort of thing.
On 1/17/2010 11:27 AM, Peter Eisentraut wrote: > I have found an Autoconf macro that checks whether the compiler properly > supports C99 inline semantics. This would allow us to replace the > __GNUC__ conditional with HAVE_C99_INLINE, in this case. At present, PostgreSQL uses only "static inline", where "inline" might be mapped to empty or to an alternate spelling via a #define generated by autoconf's AC_C_INLINE macro. static inline functions seem to work the same (modulo spelling and warnings) across many extended C89 compilers, including gcc and MSVC. The already existing de facto standard has been adopted into standard C++ and C99. Consequently, more implementations will converge toward standard "static inline" support. However, C99 introduces its own new rules for functions that are declared as inline but not static. There was never a widely accepted de facto standard for non-static inlines. Thus, older non-static inline functions typically require source changes in order to upgrade to C99. Because of this source-level upward incompatibility, compilers might activate the new rules only when compiling in C99 mode. Under the C99 non-static inline rules, the programmer provides an out-of-line definition which the compiler can call anytime it decides not to use the inline definition. A compiler might implicitly generate an out-of-line instantiation of a static inline function in every compilation unit where the definition is seen; but by using the C99 non-static inline feature, the programmer can prevent the implicit generation of out-of-line copies of the function. This is essentially an optimization done manually by the programmer instead of automatically by the compiler or linker. Do we have any need for C99 non-static inlines? I think we'll be alright if we continue to declare all of our inline functions as static. Multiple implicit out-of-line instantiations are unlikely to be a problem because: - typical inline functions are so simple that they'll always be generated inline - e.g. list_head() - compilers commonly don't generate code for a static function unless the compilation unit contains a call to the function - linkers typically delete functions that are not externally visible and are not called - linkers may eliminate duplicate sections - typical inline functions are so small that any remaining extra copies don't matter At present, IMO, we don't need full C99 inline semantics. The widely supported de facto standard "static inline" semantics, checked by the existing AC_C_INLINE test, are good enough, or nearly so. To safely expand the use of inline functions in header files across a wider set of platforms, we merely need to protect against unwanted warnings from compilation units which don't actually call all of the static inline functions defined in their included headers. Regards, ... kurt
On 1/18/2010 11:48 PM, Peter Eisentraut wrote: > On mån, 2010-01-18 at 16:34 -0800, Kurt Harriman wrote: >> MSVC does warn about unused static inline functions. The warning >> is prevented by using __forceinline instead of __inline. > > Hmm, but forceinline is not the same as inline. Are we confident that > forcing inlining is not going to lead to disadvantages? Has this been > measured? My patch uses __forceinline only in header files where it is needed to avoid warnings. Inline functions in header files are typically trivial: list_head(), MemoryContextSwitchTo(); so I expect they'll always be inlined no matter whether we use __inline or __forceinline. These are frequently called on heavily trafficked paths, so I think we want them to be inlined always. We have some existing inline functions in .c files. These can be more complicated, so it might be ok if the compiler decides to leave them out-of-line. And they are never unreferenced, so suppression of unused-function warnings is not necessary and perhaps not wanted. To leave these functions undisturbed, my patch doesn't redefine the "inline" keyword; instead it adds a new #define PG_INLINE for use in header files where unused-function warnings need to be suppressed. > Is there not a setting to disable this particular warning. I read that > MSVC has various ways to set that sort of thing. Yes, warnings can be turned off by a #pragma specifying the warning number. http://msdn.microsoft.com/en-us/library/2c8f766e%28VS.71%29.aspx http://msdn.microsoft.com/en-us/library/cw9x3tcf%28VS.100%29.aspx http://msdn.microsoft.com/en-us/library/thxezb7y%28VS.71%29.aspx To turn off unused-function warnings for just certain inline functions, one could use #ifdef _MSC_VER #pragma warning(push) #pragma warning(disable:4514) #endif ... inline function definitions ... #ifdef _MSC_VER #pragma warning(pop) #endif Or compiler switches could be set to disable all such warnings globally. Warning 4514 is specific to inline functions; so maybe it would be alright to keep it turned off globally. Note, this warning is disabled by default, but that is typically overridden by the /Wall option which changes all off-by-default warnings to become on-by-default. Probably /Wall could be combined with /Wd4514 to keep unused-inline-function warnings turned off while enabling the rest. If that is acceptable, we wouldn't need #ifdefs, #pragmas, or __forceinline. Regards, ... kurt
On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote: > On 1/18/2010 11:48 PM, Peter Eisentraut wrote: > We have some existing inline functions in .c files. These can be > more complicated, so it might be ok if the compiler decides to > leave them out-of-line. And they are never unreferenced, so > suppression of unused-function warnings is not necessary and > perhaps not wanted. To leave these functions undisturbed, my patch > doesn't redefine the "inline" keyword; instead it adds a new #define > PG_INLINE for use in header files where unused-function warnings > need to be suppressed. One principle that I suppose should have been made more explicit is that -- in my mind -- we should avoid littering our code with nonstandard constructs in place of standard constructs. Because the next generation of developers won't know what PG_INLINE is and why we're not using plain inline, even if we document it somewhere. That said, ... > > Is there not a setting to disable this particular warning. I read that > > MSVC has various ways to set that sort of thing. > > Yes, warnings can be turned off by a #pragma specifying the > warning number. > Or compiler switches could be set to disable all such warnings > globally. Warning 4514 is specific to inline functions; so > maybe it would be alright to keep it turned off globally. ... I think that would exactly be the right solution. Then just replace in those two locations __GNUC__ by __GNUC__ || __MSVC__ (or whatever the symbol is). Or if you want to make it extra nice, create a symbol somewhere like in c.h that reads #define USE_INLINE __GNUC__ || __MSVC__
Peter Eisentraut <peter_e@gmx.net> writes:
> On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote:
>> Or compiler switches could be set to disable all such warnings
>> globally. Warning 4514 is specific to inline functions; so
>> maybe it would be alright to keep it turned off globally.
> ... I think that would exactly be the right solution.
I agree that that is a better/safer approach than using __forceinline.
> Then just replace in those two locations __GNUC__ by __GNUC__ ||
> __MSVC__ (or whatever the symbol is). Or if you want to make it extra
> nice, create a symbol somewhere like in c.h that reads
> #define USE_INLINE __GNUC__ || __MSVC__
Kurt's patch proposes to try to define USE_INLINE via a configure test
rather than hard-coding it like that. While I'm not entirely convinced
that the configure test will work, I like hard-coding it even less.
Let's try the configure test and see what happens.
regards, tom lane
On 1/19/2010 8:43 AM, Tom Lane wrote: > Peter Eisentraut<peter_e@gmx.net> writes: >> On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote: >>> Or compiler switches could be set to disable all such warnings >>> globally. Warning 4514 is specific to inline functions; so >>> maybe it would be alright to keep it turned off globally. > >> ... I think that would exactly be the right solution. > > I agree that that is a better/safer approach than using __forceinline. > >> Then just replace in those two locations __GNUC__ by __GNUC__ || >> __MSVC__ (or whatever the symbol is). Or if you want to make it extra >> nice, create a symbol somewhere like in c.h that reads > >> #define USE_INLINE __GNUC__ || __MSVC__ > > Kurt's patch proposes to try to define USE_INLINE via a configure test > rather than hard-coding it like that. While I'm not entirely convinced > that the configure test will work, I like hard-coding it even less. > Let's try the configure test and see what happens. > > regards, tom lane I'll submit an updated patch. Regards, ... kurt
On Tue, Jan 19, 2010 at 4:55 PM, Kurt Harriman <harriman@acm.org> wrote: > I'll submit an updated patch. We need this patch pretty soon if we're going to include this in 9.0, I think. ...Robert
Revised patch is attached (3rd edition). It's also available in my git repository in the "submitted" branch: http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted With this patch, the "configure" script tests whether a static inline function can be defined without incurring a warning when not referenced. If successful, the preprocessor symbol inline_quietly is defined in pg_config.h to the appropriate keyword: inline, __inline, or __inline__. Otherwise inline_quietly remains undefined. palloc.h and pg_list.h condition their inline function definitions on inline_quietly instead of the gcc-specific __GNUC__. Thus the functions can be inlined on more platforms, not only gcc. Ordinary out-of-line calls are still used if the compiler doesn't recognize inline functions, or spews warnings when static inline functions are defined but not referenced. Changes since the previous edition of this patch: - Renamed the new preprocessor symbol to "inline_quietly" instead of PG_INLINE. inline_quietly is more descriptive, and shows up when grepping for "inline". - Removed MSVC-related changes (__forceinline) from the autoconf stuff. Instead, updated the manually-edited pg_config.h.win32 file to define both "inline" and "inline_quietly" as __inline. - Removed Windows-only misspelling of __inline__ in instr_time.h. This was the only occurrence of __inline__; therefore, deleted the no-longer-needed definition of __inline__ from port/win32.h. Also deleted the definition of inline from port/win32.h, since it is now defined in pg_config.h.win32, consistent with the other platforms. Thanks to all who commented. Regards, ... kurt
Attachment
On 1/18/2010 4:42 PM, Tom Lane wrote: > I think including MSVC in the set of compilers targeted by a configure > test is just a waste of code. It's more likely to confuse people than > help them. Ok, I have taken that out, and instead put the right stuff for MSVC into pg_config.h.win32. Regards, ... kurt
On 1/18/2010 11:48 PM, Peter Eisentraut wrote: > On mån, 2010-01-18 at 16:34 -0800, Kurt Harriman wrote: >> MSVC does warn about unused static inline functions. The warning >> is prevented by using __forceinline instead of __inline. > > Hmm, but forceinline is not the same as inline. Are we confident that > forcing inlining is not going to lead to disadvantages? Has this been > measured? > > Is there not a setting to disable this particular warning. I read that > MSVC has various ways to set that sort of thing. On further investigation, plain __inline works alright, at least in Visual Studio 2005. The warning for unused static inline functions only comes up when compiling C++, not C. I've submitted a revised patch, no longer using __forceinline. Regards, ... kurt
On 1/19/2010 8:01 AM, Peter Eisentraut wrote: > On tis, 2010-01-19 at 01:29 -0800, Kurt Harriman wrote: >> On 1/18/2010 11:48 PM, Peter Eisentraut wrote: >> We have some existing inline functions in .c files. These can be >> more complicated, so it might be ok if the compiler decides to >> leave them out-of-line. And they are never unreferenced, so >> suppression of unused-function warnings is not necessary and >> perhaps not wanted. To leave these functions undisturbed, my patch >> doesn't redefine the "inline" keyword; instead it adds a new #define >> PG_INLINE for use in header files where unused-function warnings >> need to be suppressed. > > One principle that I suppose should have been made more explicit is that > -- in my mind -- we should avoid littering our code with nonstandard > constructs in place of standard constructs. Because the next generation > of developers won't know what PG_INLINE is and why we're not using plain > inline, even if we document it somewhere. Unfortunately, to switch to an out-of-line function where inlining is not supported, a separate preprocessor symbol is needed. The already existing "inline" define can't be used to test whether the compiler supports inlining. "inline" is defined as empty if configure doesn't detect an acceptable variant of "inline". It is left undefined if the compiler accepts the standard spelling "inline". But the C preprocessor offers no way to test whether a symbol is defined as empty. #if can compare integers but not strings. Everyone seems to hate the name PG_INLINE, so I've changed it to inline_quietly, which is more descriptive. Anyone who greps for the definition of inline_quietly will find the comment in pg_config.h telling what it is and how it should be used, as well as the examples in pg_list.h and palloc.h. Also it is explained, I hope clearly, in the proposed CVS commit comment and in this email thread. > Then just replace in those two locations __GNUC__ by __GNUC__ || > __MSVC__ (or whatever the symbol is). Or if you want to make it extra > nice, create a symbol somewhere like in c.h that reads > > #define USE_INLINE __GNUC__ || __MSVC__ That would just add to the compiler-specific preprocessor logic to be duplicated in every header file in which inline functions are defined. I'm trying to factor out that compiler dependency into a central location: pg_config.h. Regards, ... kurt
On 12/16/2009 8:40 AM, Tom Lane wrote: > Alvaro Herrera<alvherre@commandprompt.com> writes: >> IIRC Kurt was also on about getting rid of some ugly macros that could >> instead be coded as inline functions (fastgetattr for example) > > I'd just bounce that as useless activity. If they are macros now, > and work, the only possible effects of changing them are negative. fastgetattr has just been changed by Robert Haas on 10 Jan 2010: "Remove partial, broken support for NULL pointers when fetching attributes." Changing fastgetattr to an inline function would make it - easier to read, modify, and review for correctness - debuggable: could set breakpoints, single-step, display the arguments - profilable and would make compiler warnings appear at the definition rather than at every invocation. Regards, ... kurt
On 12/16/2009 8:21 AM, Tom Lane wrote: > I would only suggest that the cleanest coding would be > > #ifdef USE_INLINE > > static inline foo(...) ... > > #else > > ... non-inline definition of foo > > #endif > > ie, go ahead and rely on autoconf's definition (if any) of "inline" > and add a policy symbol USE_INLINE to determine whether to use it. That would work for gcc and MSVC. But it wouldn't allow for configuring an alternative keyword (like __forceinline) or added magic (like inserting an __attribute__ or __declspec) to silence warnings for some compiler which we don't know about yet. > The proposed PG_INLINE coding conflates the symbol needed in the code > with the policy choice. Everyone is familiar with this idiom: first test whether a pointer is NULL, before dereferencing it. We don't use a separate flag to say whether the pointer is NULL. > Another possibility would be to call the policy symbol HAVE_INLINE, > but that (a) risks collision with a name defined by autoconf built-in > macros, and (b) looks like it merely indicates whether the compiler > *has* inline, not that we have made a choice about how to use it. In the new 3rd edition of the patch, I've changed the name to inline_quietly. If not too many people hate this new name, I can undertake a new career naming tablets for Apple. :) Regards, ... kurt
Kurt Harriman <harriman@acm.org> writes:
> On 1/19/2010 8:01 AM, Peter Eisentraut wrote:
>> One principle that I suppose should have been made more explicit is that
>> -- in my mind -- we should avoid littering our code with nonstandard
>> constructs in place of standard constructs.
> Everyone seems to hate the name PG_INLINE, so I've changed it to
> inline_quietly, which is more descriptive.
Kurt, you seem to be more or less impervious to advice :-(.
Please just make the thing be "inline" and have configure define
USE_INLINE, as per previous discussion. Cluttering the code with
nonstandard constructs is not good for readability. More, it is likely
to confuse syntax-aware editors and pgindent, neither of which will read
any of the discussion or commit messages.
regards, tom lane
Revised patch is attached (4th edition). It's also available in my git repository in the "submitted" branch: http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted With this patch, the "configure" script tests whether a static inline function can be defined without incurring a warning when not referenced. If successful, the preprocessor symbol USE_INLINE is defined to 1 in pg_config.h. Otherwise USE_INLINE remains undefined. palloc.h and pg_list.h condition their inline function definitions on USE_INLINE instead of the gcc-specific __GNUC__. Thus the functions can be inlined on more platforms, not only gcc. Ordinary out-of-line calls are still used if the compiler doesn't recognize inline functions, or spews warnings when static inline functions are defined but not referenced. From now on in cross-platform code, plain "inline" should be used instead of compiler-specific alternate spellings such as __inline__, because the configure script redefines "inline" to the appropriate alternate spelling if necessary. (This patch doesn't affect the redefinition of "inline": that was already in place.) Changes since the second and third editions of this patch: - Renamed the new preprocessor symbol to USE_INLINE, following the advice of Tom Lane, instead of my earlier attempts PG_INLINE or inline_quietly. It is used like this: /* include/utils/palloc.h */ #ifdef USE_INLINE static inline MemoryContext MemoryContextSwitchTo(MemoryContext context) { MemoryContext old = CurrentMemoryContext; CurrentMemoryContext = context; return old; } #else extern MemoryContext MemoryContextSwitchTo(MemoryContext context); #endif /* USE_INLINE */ /* backend/utils/mmgr/mcxt.c */ #ifndef USE_INLINE MemoryContext MemoryContextSwitchTo(MemoryContext context) { MemoryContext old; AssertArg(MemoryContextIsValid(context)); old = CurrentMemoryContext; CurrentMemoryContext = context; return old; } #endif /* ! USE_INLINE */ - Simplified the autoconf logic added to config/c-compiler.m4. - Removed MSVC-related changes (__forceinline) from the autoconf stuff. Instead, updated the manually-edited pg_config.h.win32 file to define "inline" as __inline, and USE_INLINE as 1. - Removed Windows-only misspelling of __inline__ in instr_time.h. This was the only occurrence of __inline__; therefore, deleted the no-longer-needed definition of __inline__ from port/win32.h. Also deleted the definition of inline from port/win32.h, since it is now defined in pg_config.h.win32, consistent with the other platforms. Thanks again to all who have commented. Regards, ... kurt
Attachment
On 2/10/2010 7:12 AM, Tom Lane wrote: > Kurt, you seem to be more or less impervious to advice :-(. Thank you for reviewing my patch. It is a rare honor to have my personal qualities reviewed here as well. Since this forum is archived for posterity, I suppose I must point out that I have in fact been responsive to all the advice that has been offered here. I have answered each comment fully and politely. I have acted upon most of the suggestions, and have revised my small patch accordingly and resubmitted it twice (now thrice, incorporating this comment of yours). Admittedly I have used my own judgment in how to adopt these sometimes conflicting suggestions; and have explained the rationale for my choices. Everyone may judge whether my choices and explanations are satisfactory and continue the dialogue if they are not yet satisfied. By sincerely engaging in such a process, consensus may be reached. All contributors to the pg_hackers list are well advised to be impervious to brusque and sometimes rude dismissals, which seem to be de rigueur here. However, the evidence of this thread shows that I have been far from impervious to advice. By the way, suggestions which must be carried out without question are "orders", not "advice". When a statement, meant to be imperative, is phrased softly as advice, it can easily be mistaken as optional by newcomers who may not have fully grasped the prevailing reality. Thus, commands are best stated in clear language. > Please just make the thing be "inline" and have configure define > USE_INLINE, as per previous discussion. Just now I have resubmitted according to your instruction. > Cluttering the code with nonstandard constructs is not> good for readability. Agreed. But any program consists of definitions of new identifiers, data structures, macros, and conventions or guidelines for their use. What, or who, differentiates ordinary programming practice, such as typedefs, from "nonstandard constructs"? > More, it is likely to confuse syntax-aware editors and> pgindent, neither of which will read any of the discussion> orcommit messages. Good point. This had not been mentioned before. It works alright with the syntax-aware editors that I use, and I haven't had occasion to run pgindent, so I didn't think of this earlier. Does the same problem exist with the PGDLLIMPORT macro defined in postgres.h? It, too, is used in the same syntactic niche where "inline" would be placed. Regards, ... kurt
On Wed, Feb 10, 2010 at 5:53 PM, Kurt Harriman <harriman@acm.org> wrote: > Revised patch is attached (4th edition). This looks generally sane to me, though it seems entirely imaginable that it might break something, somewhere, for somebody... in which case I suppose we'll adjust as needed. Peter, are you planning to commit this one? ...Robert
On Wed, Feb 10, 2010 at 6:04 PM, Kurt Harriman <harriman@acm.org> wrote: > By the way, suggestions which must be carried out without > question are "orders", not "advice". When a statement, > meant to be imperative, is phrased softly as advice, it can > easily be mistaken as optional by newcomers who may not have > fully grasped the prevailing reality. Thus, commands are > best stated in clear language. I think the reason people tend to phrase things in terms of opinions or advice is because no single person here is able to speak with complete authority, and those who attempt to do so tend to draw irritated responses when a softer statement would have passed unchallenged. At the same time, when two or three people all express an opinion that we should do X, and especially when some of those people are committers, it's very difficult to get a patch committed that does not-X. Maybe this isn't as obvious to newcomers as it could be, although it's not entirely clear to me how we could make it so without being heavy-handed about it... it's not that you can never win an argument of this type; it's just that you need a darn good reason and some allies, and it's hard to come up with darn good reasons when the discussion is basically about style. Having said all that, I sympathize with your frustration; I've been there a few times myself. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Feb 10, 2010 at 5:53 PM, Kurt Harriman <harriman@acm.org> wrote:
>> Revised patch is attached (4th edition).
> This looks generally sane to me, though it seems entirely imaginable
> that it might break something, somewhere, for somebody... in which
> case I suppose we'll adjust as needed.
Looks sane to me too -- committed. We'll soon see what the buildfarm
thinks (though the number of non-gcc buildfarm members is depressingly
small).
regards, tom lane
On Fri, Feb 12, 2010 at 9:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Feb 10, 2010 at 5:53 PM, Kurt Harriman <harriman@acm.org> wrote: >>> Revised patch is attached (4th edition). > >> This looks generally sane to me, though it seems entirely imaginable >> that it might break something, somewhere, for somebody... in which >> case I suppose we'll adjust as needed. > > Looks sane to me too -- committed. We'll soon see what the buildfarm > thinks (though the number of non-gcc buildfarm members is depressingly > small). I can't feel bad about the near-ubiquity of gcc... life would be a lot harder if it were otherwise. ...Robert