Обсуждение: pgsql: Move Assert() definitions to c.h
Move Assert() definitions to c.h This way, they can be used by frontend and backend code. We already supported that, but doing it this way allows us to mix true frontend files with backend files compiled in frontend environment. Author: Andres Freund Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/e1d25de35a2b1f809e8f8d7b182ce0af004f3ec9 Modified Files -------------- src/include/c.h | 195 +++++++++++++++++++++++++++++++-------------- src/include/postgres.h | 54 +------------ src/include/postgres_fe.h | 12 --- 3 files changed, 136 insertions(+), 125 deletions(-)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Move Assert() definitions to c.h
This seems a bit odd. Why didn't you move all of postgres.h's section 3
into the new section in c.h? The couple of declarations you left there
are neither useful nor intelligible on their own.
Also, I think there's a typo here:
+#else /* USE_ASSERT_CHECKING && FRONTEND */
Should be
+#else /* USE_ASSERT_CHECKING && !FRONTEND */
no?
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > Move Assert() definitions to c.h > > This seems a bit odd. Why didn't you move all of postgres.h's section 3 > into the new section in c.h? The couple of declarations you left there > are neither useful nor intelligible on their own. Hmm, true, I didn't consider that. Will fix. > Also, I think there's a typo here: > > +#else /* USE_ASSERT_CHECKING && FRONTEND */ > > Should be > > +#else /* USE_ASSERT_CHECKING && !FRONTEND */ > > no? This bit I did consider. It wasn't clear to me whether those comments mean that the condition spelled out in the comment holds for the lines below, or that they hold for the lines above. Note the comment after the #endif holds for the lines above it. After searching for precedent, I found one in ip.c and it seems to support what you say. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2013-02-01 19:44:42 -0300, Alvaro Herrera wrote: > Tom Lane wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > Move Assert() definitions to c.h > > > > This seems a bit odd. Why didn't you move all of postgres.h's section 3 > > into the new section in c.h? The couple of declarations you left there > > are neither useful nor intelligible on their own. > > Hmm, true, I didn't consider that. Will fix. I left it there because assert_enabled and ExceptionalCondition should never be required in a frontend environment. Wasn't sure about that one, so moving it is absolutely fine with me. > > > > Also, I think there's a typo here: > > > > +#else /* USE_ASSERT_CHECKING && FRONTEND */ > > > > Should be > > > > +#else /* USE_ASSERT_CHECKING && !FRONTEND */ > > > > no? > > This bit I did consider. It wasn't clear to me whether those comments > mean that the condition spelled out in the comment holds for the lines > below, or that they hold for the lines above. Note the comment after > the #endif holds for the lines above it. > > After searching for precedent, I found one in ip.c and it seems to > support what you say. I think there exists precedent for both spellings, don't really about which one we use here. One other thing I didn't think of, if youre at it agan, what about changing the #ifndef USE_ASSERT_CHECKING definition of #define Assert(condition) into #define Assert(condition) ((void)true) That makes for fewer warnings on some pedantic compiler modes and is in line with C's assert() IIRC. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > Move Assert() definitions to c.h > > This seems a bit odd. Why didn't you move all of postgres.h's section 3 > into the new section in c.h? The couple of declarations you left there > are neither useful nor intelligible on their own. One slight problem in this line: extern PGDLLIMPORT bool assert_enabled; This appears in line 622 in c.h, but PGDLLIMPORT is not defined till line 965. So we would have to put that declaration below section 9 "system specific hacks". Having a separate #if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND) section seems weird. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> This seems a bit odd. Why didn't you move all of postgres.h's section 3
>> into the new section in c.h? The couple of declarations you left there
>> are neither useful nor intelligible on their own.
> One slight problem in this line:
> extern PGDLLIMPORT bool assert_enabled;
> This appears in line 622 in c.h, but PGDLLIMPORT is not defined till
> line 965. So we would have to put that declaration below section 9
> "system specific hacks". Having a separate
> #if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND)
> section seems weird.
Ah. Okay, let's leave it alone then. It might be worth adding a
comment though that says these declarations are support for the
Assert-related macros in c.h.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Tom Lane wrote: > >> This seems a bit odd. Why didn't you move all of postgres.h's section 3 > >> into the new section in c.h? The couple of declarations you left there > >> are neither useful nor intelligible on their own. > > > One slight problem in this line: > > > extern PGDLLIMPORT bool assert_enabled; > > > This appears in line 622 in c.h, but PGDLLIMPORT is not defined till > > line 965. So we would have to put that declaration below section 9 > > "system specific hacks". Having a separate > > #if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND) > > section seems weird. > > Ah. Okay, let's leave it alone then. It might be worth adding a > comment though that says these declarations are support for the > Assert-related macros in c.h. Fixed; I did move the other declaration to c.h. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services