Thread: [HACKERS] datetime.h defines like PM conflict with external libraries
Hi, In my llvm jit work I'd to #undef PM /* include some llvm headers */ #define PM 1 because llvm has a number of functions which have an argument named PM. Now that works, but it's fairly ugly. Perhaps it would be a good idea to name these defines in a manner that's slightly less likely to conflict? Alternatively we could use #pragma push_macro() around the includes, but that'd be a new dependency. Better ideas? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 10/03/2017 03:00 PM, Andres Freund wrote: > Hi, > > In my llvm jit work I'd to > > #undef PM > /* include some llvm headers */ > #define PM 1 > > because llvm has a number of functions which have an argument named PM. > Now that works, but it's fairly ugly. Perhaps it would be a good idea to > name these defines in a manner that's slightly less likely to conflict? > > Alternatively we could use #pragma push_macro() around the includes, but > that'd be a new dependency. > > Better ideas? > AFAICT at a quick glance these are only used in a couple of files. Maybe the defs need to be floated off to a different header with more limited inclusion? cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Hi, On 2017-10-03 16:34:38 -0400, Andrew Dunstan wrote: > On 10/03/2017 03:00 PM, Andres Freund wrote: > > Hi, > > > > In my llvm jit work I'd to > > > > #undef PM > > /* include some llvm headers */ > > #define PM 1 > > > > because llvm has a number of functions which have an argument named PM. > > Now that works, but it's fairly ugly. Perhaps it would be a good idea to > > name these defines in a manner that's slightly less likely to conflict? > > > > Alternatively we could use #pragma push_macro() around the includes, but > > that'd be a new dependency. > > > > Better ideas? > AFAICT at a quick glance these are only used in a couple of files. Maybe > the defs need to be floated off to a different header with more limited > inclusion? Why not just rename them to PG_PM etc? If we force potential external users to do some changes, we can use more unique names just as well - the effort to adapt won't be meaningfully higher... IMNSHO there's not much excuse for defining macros like PM globally. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
> On 2017-10-03 16:34:38 -0400, Andrew Dunstan wrote:
>> AFAICT at a quick glance these are only used in a couple of files. Maybe
>> the defs need to be floated off to a different header with more limited
>> inclusion?
> Why not just rename them to PG_PM etc? If we force potential external
> users to do some changes, we can use more unique names just as well -
> the effort to adapt won't be meaningfully higher... IMNSHO there's not
> much excuse for defining macros like PM globally.
I like the new-header-file idea because it will result in minimal code
churn and thus minimal back-patching hazards.
I do *not* like "PG_PM". For our own purposes that adds no uniqueness
at all. If we're to touch these symbols then I'd go for names like
"DATETIME_PM". Or maybe "DT_PM" ... there's a little bit of precedent
for the DT_ prefix already.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/03/2017 04:43 PM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> On 2017-10-03 16:34:38 -0400, Andrew Dunstan wrote: >>> AFAICT at a quick glance these are only used in a couple of files. Maybe >>> the defs need to be floated off to a different header with more limited >>> inclusion? >> Why not just rename them to PG_PM etc? If we force potential external >> users to do some changes, we can use more unique names just as well - >> the effort to adapt won't be meaningfully higher... IMNSHO there's not >> much excuse for defining macros like PM globally. > I like the new-header-file idea because it will result in minimal code > churn and thus minimal back-patching hazards. > > I do *not* like "PG_PM". For our own purposes that adds no uniqueness > at all. If we're to touch these symbols then I'd go for names like > "DATETIME_PM". Or maybe "DT_PM" ... there's a little bit of precedent > for the DT_ prefix already. > > Yeah. If we use a prefix +1 for DT_. If we do that then I think they should *all* be prefixed, not just the ones we know of conflicts for. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Andrew Dunstan wrote: > On 10/03/2017 04:43 PM, Tom Lane wrote: > > I like the new-header-file idea because it will result in minimal code > > churn and thus minimal back-patching hazards. > > > > I do *not* like "PG_PM". For our own purposes that adds no uniqueness > > at all. If we're to touch these symbols then I'd go for names like > > "DATETIME_PM". Or maybe "DT_PM" ... there's a little bit of precedent > > for the DT_ prefix already. > > Yeah. If we use a prefix +1 for DT_. If we do that then I think they > should *all* be prefixed, not just the ones we know of conflicts for. Maybe it'd be good idea to unify some of that stuff so that ecpg can use it, too? Having a second copy of the same stuff in src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible. Even if not, let's make sure they don't diverge. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
> Maybe it'd be good idea to unify some of that stuff so that ecpg can > use > it, too? Having a second copy of the same stuff in > src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible. Even if not, > let's make sure they don't diverge. Please let's unify whatever we can. The fewer manual sync we need, the better. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Michael Meskes <meskes@postgresql.org> writes:
>> Maybe it'd be good idea to unify some of that stuff so that ecpg can
>> use it, too? Having a second copy of the same stuff in
>> src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible. Even if not,
>> let's make sure they don't diverge.
> Please let's unify whatever we can. The fewer manual sync we need, the
> better.
Isn't pgtypeslib/*.h exposed to ecpg-using applications?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
> Isn't pgtypeslib/*.h exposed to ecpg-using applications? No, the public interface is is include/*.h, pgtypeslib/*.h is only internal. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
On 2017-10-04 11:36:56 +0200, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
> > On 10/03/2017 04:43 PM, Tom Lane wrote:
> > > I like the new-header-file idea because it will result in minimal code
> > > churn and thus minimal back-patching hazards.
I'm not sure it's that little code churn, and the insulation isn't
great. Based on my WIP patch adding a DT_ prefix it would affect at
least:
contrib/adminpack/adminpack.c | 8 +-
src/backend/parser/gram.y | 52 ++++-----
src/backend/utils/adt/date.c | 50 ++++----
src/backend/utils/adt/datetime.c | 614
+++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
src/backend/utils/adt/formatting.c | 10 +-
src/backend/utils/adt/json.c | 8 +-
src/backend/utils/adt/nabstime.c | 32 +++---
src/backend/utils/adt/timestamp.c | 196 +++++++++++++++----------------
src/backend/utils/adt/xml.c | 6 +-
src/backend/utils/misc/tzparser.c | 4 +-
src/bin/pg_waldump/compat.c | 6 +-
src/include/utils/datetime.h | 216 +++++++++++++++++------------------
so I'm not quite convinced this that well isolated pieces of code. I
wonder if just moving the defines around won't primarily increase pain.
I have however, for now, worked around the need to deal with this
problem (by moving more stuff .c files that are careful about their
includes). So this is more about historical raisins, I do not have an
urgent need to work on this.
> > > I do *not* like "PG_PM". For our own purposes that adds no uniqueness
> > > at all. If we're to touch these symbols then I'd go for names like
> > > "DATETIME_PM". Or maybe "DT_PM" ... there's a little bit of precedent
> > > for the DT_ prefix already.
> >
> > Yeah. If we use a prefix +1 for DT_. If we do that then I think they
> > should *all* be prefixed, not just the ones we know of conflicts for.
Attached is a WIP patch doing exactly this conversion for
datetime.h. Note that we'd want to do something about ecpg's dt.h if we
were to go for the approach.
While the changes are fairly verbose, they're also mechanical, so I
suspect the issues around backpatching - not that that code changes that
much - wouldn't be too hard to resolve.
> Maybe it'd be good idea to unify some of that stuff so that ecpg can use
> it, too? Having a second copy of the same stuff in
> src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible. Even if not,
> let's make sure they don't diverge.
I agree that that would be quite an advantage. It's more than just
datetime.h that'd need to be usable by ecpg. Luckily timestamp.h would
probably be easy,
commit a7801b62f21bd051444bd1119cd3745ecc8e14ec
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: 2011-09-09 13:23:41 -0400
Move Timestamp/Interval typedefs and basic macros into datatype/timestamp.h.
provides the basics. I suspect we'd want to do something very similar
for datetime?
I however wonder if even that would be really going far enough - we'd
still end up with a lot of copied functions:
int DecodeInterval(char **, int *, int, int *, struct tm *, fsec_t *);
int DecodeTime(char *, int *, struct tm *, fsec_t *);
void EncodeDateTime(struct tm *tm, fsec_t fsec, bool print_tz, int tz, const char *tzn, int style, char *str,
boolEuroDates);
void EncodeInterval(struct tm *tm, fsec_t fsec, int style, char *str);
int tm2timestamp(struct tm *, fsec_t, int *, timestamp *);
int DecodeUnits(int field, char *lowtoken, int *val);
bool CheckDateTokenTables(void);
void EncodeDateOnly(struct tm *tm, int style, char *str, bool EuroDates);
int GetEpochTime(struct tm *);
int ParseDateTime(char *, char *, char **, int *, int *, char **);
int DecodeDateTime(char **, int *, int, int *, struct tm *, fsec_t *, bool);
void j2date(int, int *, int *, int *);
void GetCurrentDateTime(struct tm *);
int date2j(int, int, int);
void TrimTrailingZeros(char *);
void dt2time(double, int *, int *, int *, fsec_t *);
I suspect starting to implement infrastructure to deal with would be a
bit bigger a task than I can chew of right now though. Medium term, it
seems to me, we should start actually move a lot of the adt code into a
library that can be included (or possibly just compiled?) both by
frontend and backend code. Which kinda seems to imply we'd need
compatible elog support for frontend code, which I'd wished for many
times.
Michael, is there any problem including datatype/* headers in ecpg that
are frontend clean? I see no such usage so far, that's why I'm asking.
Greetings,
Andres Freund
Attachment
> Michael, is there any problem including datatype/* headers in ecpg > that > are frontend clean? I see no such usage so far, that's why I'm > asking. When the pgtypes library was created we tried to include only the bits and pieces needed to not create unnecessary hassles, but if it compiles cleanly I'm fine either way. I'm assuming you're talking about including the files for compiling ecpg, not as externally visible header files, right? michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Andres Freund wrote: > Medium term, it > seems to me, we should start actually move a lot of the adt code into a > library that can be included (or possibly just compiled?) both by > frontend and backend code. Which kinda seems to imply we'd need > compatible elog support for frontend code, which I'd wished for many > times. I remember looking into moving this code into src/common/ a couple of years ago, but the API was not identical in ecpg than backend mostly because of the use of a few GUC vars, so I didn't finish it. In many cases it seemed possible to resolve easily (just change hardcoded use of a GUC in the function with a parameter), but I'm not sure it was the case everywhere. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services