Re: [GENERAL] C++ port of Postgres

On Thu, Sep 1, 2016 at 1:41 AM, Peter Eisentraut <
peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:

> [trimmed cc list because of big attachments]
>
> On 8/16/16 4:22 PM, Jim Nasby wrote:
> > Joy, do you have an idea what a *minimally invasive* patch for C++
> > support would look like? That's certainly the first step here.
>
> I developed a minimally invasive patch for C++ support a few years ago
> shortly after I wrote that blog post. Since there appears to have been
> some interest here now, I have updated that and split it up into logical
> chunks.
>
> So here you go.
>

I looked at a random selection of these patches this morning.

> 0001-Fix-use-of-offsetof.patch

I agree that this is already invalid C because it's not constant. It is
accepted by GCC's C front end no matter what switches you give it and
clearly many other compilers too, but not clang's C front end with
"-pedantic".

> 0002-Use-return-instead-of-exit-in-configure.patch

Makes sense. Any reason not to #include <stdlib.h> instead like you did
for the 0003 patch?

> 0003-Add-missing-include-files-to-configure-tests.patch

Makes sense.

> 0014-Set-up-for-static-asserts-in-C.patch

+#if __cpp_static_assert >= 201411
+#define _Static_assert(condition, errmessage) static_assert(condition,
errmessage)

Don't you mean 201103L? C++11 introduced two-argument static_assert, not
C++14.

> 0021-Workaround-for-using-typdef-ed-ints-in-loops.patch
>
> Types made from int don't have a ++ operator in C++, so they can't be
> used in for loops without further work.

ForkNumber is not a typedef'd int: it's an enum. Since it's called a "fork
*number*" and clearly treated as a number in various places including loops
that want to increment it, perhaps it really should be a typedef of an int,
instead of an enum. Then either macros or an enum ForkNumberEnum could
define the names (you can assign enums to ints, and compare enums and ints,
you just can't assign ints to enums so it'd be no problem to have typedef
ForkNumber int and then enum ForkNumberEnum { ... } to define the friendly
names, but never actually use the type ForkNumberEnum).

> 0023-Add-C-linkage-to-replacement-declaration-of-fdatasyn.patch

-extern int fdatasync(int fildes);
+extern "C" int fdatasync(int fildes);

Doesn't this need to be made conditional on __cplusplus?

> 0024-Make-inet_net_-to-C-linkage.patch

Same.

> 0025-Add-C-linkage-to-functions-exported-by-plugins.patch

-extern void _PG_init(void);
+extern "C" void _PG_init(void);

Why this way in some places...

+extern "C" {
extern void _PG_init(void);
+}

... and this way in single-function declaration cases in other places?

-char *widget_out(WIDGET *widget);
+char *widget_out(WIDGET * widget);

Noise.

> 0027-Hack-Disable-volatile-that-causes-mysterious-compile.patch
>
> Subject: [PATCH 27/27] Hack: Disable volatile that causes mysterious
compiler errors
I don't grok the reason for the volatile qualifier in this code the first
place but if it actually does something useful, here's one way to fix it.
The specific reason for the error reported by GCC is that QueuePosition (a
struct) is copied by assignment:

pos = oldpos = QUEUE_BACKEND_POS(MyBackendId);

That means that it invokes the default assignment operator, and you can't
do that with a volatile and a non-volatile object either way around
according to C++ (though clang seems less fussy than GCC in this case).
You could try to untangle that by supplying a suitably qualified explicit
member operator:

#ifdef __cplusplus

QueuePosition& operator=(const volatile QueuePosition& other)

{

page = other.page;

offset = other.offset;

return *this;

}

#endif

But that doesn't help with assignments going the other way.... How about
just defining a macro in the style of the existing macros and then using
that in place of all those incompatible assignment operations:

iff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 716f1c3..469018f 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -194,6 +194,12 @@ typedef struct QueuePosition
(x).offset = (z); \
} while (0)

+#define COPY_QUEUE_POS(x, y) \
+ do { \
+ (x).page = (y).page; \
+ (x).offset = (y).offset; \
+ } while (0)
+
#define QUEUE_POS_EQUAL(x,y) \
((x).page == (y).page && (x).offset == (y).offset)

@@ -1757,7 +1763,8 @@ asyncQueueReadAllNotifications(void)
LWLockAcquire(AsyncQueueLock, LW_SHARED);
/* Assert checks that we have a valid state entry */
Assert(MyProcPid == QUEUE_BACKEND_PID(MyBackendId));
- pos = oldpos = QUEUE_BACKEND_POS(MyBackendId);
+ COPY_QUEUE_POS(pos, QUEUE_BACKEND_POS(MyBackendId));
+ COPY_QUEUE_POS(oldpos, pos);
head = QUEUE_HEAD;
LWLockRelease(AsyncQueueLock);

@@ -1861,7 +1868,7 @@ asyncQueueReadAllNotifications(void)
{
/* Update shared state */
LWLockAcquire(AsyncQueueLock, LW_SHARED);
- QUEUE_BACKEND_POS(MyBackendId) = pos;
+ COPY_QUEUE_POS(QUEUE_BACKEND_POS(MyBackendId), pos);
advanceTail = QUEUE_POS_EQUAL(oldpos, QUEUE_TAIL);
LWLockRelease(AsyncQueueLock);

@@ -1875,7 +1882,7 @@ asyncQueueReadAllNotifications(void)

/* Update shared state */
LWLockAcquire(AsyncQueueLock, LW_SHARED);
- QUEUE_BACKEND_POS(MyBackendId) = pos;
+ COPY_QUEUE_POS(QUEUE_BACKEND_POS(MyBackendId), pos);
advanceTail = QUEUE_POS_EQUAL(oldpos, QUEUE_TAIL);
LWLockRelease(AsyncQueueLock);

@@ -1911,7 +1918,9 @@ asyncQueueProcessPageEntries(volatile QueuePosition
*current,

do
{
- QueuePosition thisentry = *current;
+ QueuePosition thisentry;
+
+ COPY_QUEUE_POS(thisentry, *current);

if (QUEUE_POS_EQUAL(thisentry, stop))
break;
@@ -1944,7 +1953,7 @@ asyncQueueProcessPageEntries(volatile QueuePosition
*current,
* from a transaction that is not yet
visible to snapshots;
* compare the comments at the head of
tqual.c.
*/
- *current = thisentry;
+ COPY_QUEUE_POS(*current, thisentry);
reachedStop = true;
break;
}

--
Thomas Munro
http://www.enterprisedb.com