Prev: [HACKERS] Git out of sync vs. CVS
Next: Git out of sync vs. CVS
From: Kurt Harriman on 10 Feb 2010 03:53
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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Kurt Harriman on 10 Feb 2010 04:00
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


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Kurt Harriman on 10 Feb 2010 04:26
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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Kurt Harriman on 10 Feb 2010 04:43
On 12/16/2009 8:40 AM, Tom Lane wrote:
> Alvaro Herrera<alvherre(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Kurt Harriman on 10 Feb 2010 05:17
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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: [HACKERS] Git out of sync vs. CVS
Next: Git out of sync vs. CVS