Re: [GENERAL] C++ port of Postgres - Mailing list pgsql-hackers

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;