Thread: [HACKERS] static assertions in C++
Commit df1a699e5ba3232f373790b2c9485ddf720c4a70 introduced a StaticAssertStmt() into a header file, which will fail if a module written in C++ uses that header file. Currently, that header file is not widely used, but it's a potential problem if the use of static assertions expands. As discussed in <https://www.postgresql.org/message-id/7775.1492448671@sss.pgh.pa.us>, a more general solution would be to add specific C++ support for static assertions in c.h. Here is a patch for that, extracted from my previously posted C++ patch set, but also a bit reworked from what was previously posted. Also attached is a little C++ test file that one can use to test this out. (Just compiling it should cause a compiler error without the patch and a static assertion failure with the patch.) -- Peter Eisentraut http://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
Attachment
On Thu, Aug 31, 2017 at 4:43 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Commit df1a699e5ba3232f373790b2c9485ddf720c4a70 introduced a > StaticAssertStmt() into a header file, which will fail if a module > written in C++ uses that header file. Currently, that header file is > not widely used, but it's a potential problem if the use of static > assertions expands. > > As discussed in > <https://www.postgresql.org/message-id/7775.1492448671@sss.pgh.pa.us>, a > more general solution would be to add specific C++ support for static > assertions in c.h. Here is a patch for that, extracted from my > previously posted C++ patch set, but also a bit reworked from what was > previously posted. I like the concept of being more C++-compatible, but I'm not sure about the idea of not providing a workaround, given that we went to the extreme of doing this: #define StaticAssertStmt(condition, errmessage) \ ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; })) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Aug 31, 2017 at 4:43 PM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> As discussed in >> <https://www.postgresql.org/message-id/7775.1492448671@sss.pgh.pa.us>, a >> more general solution would be to add specific C++ support for static >> assertions in c.h. Here is a patch for that, extracted from my >> previously posted C++ patch set, but also a bit reworked from what was >> previously posted. > I like the concept of being more C++-compatible, but I'm not sure > about the idea of not providing a workaround, Meh. We support ancient versions of C for backwards compatibility reasons, but considering that compiling backend code with C++ isn't officially supported at all, I'm not sure we need to cater to ancient C++ compilers. We could quibble about the value of "ancient" of course --- Peter, do you have an idea when this construct became widely supported? I do think it might be a better idea to put a #error there instead of silently disabling static assertions. Then at least we could hope to get complaints if anyone *is* trying to use ancient C++, and thereby gauge whether it's worth working any harder for this. regards, tom lane
On Thu, Aug 31, 2017 at 6:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Meh. We support ancient versions of C for backwards compatibility > reasons, but considering that compiling backend code with C++ isn't > officially supported at all, I'm not sure we need to cater to ancient > C++ compilers. We could quibble about the value of "ancient" of > course --- Peter, do you have an idea when this construct became > widely supported? > > I do think it might be a better idea to put a #error there instead > of silently disabling static assertions. Then at least we could > hope to get complaints if anyone *is* trying to use ancient C++, > and thereby gauge whether it's worth working any harder for this. I guess my question was whether we couldn't just use the same workaround we use for old C compilers. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Sep 1, 2017 at 8:28 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Aug 31, 2017 at 6:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Meh. We support ancient versions of C for backwards compatibility >> reasons, but considering that compiling backend code with C++ isn't >> officially supported at all, I'm not sure we need to cater to ancient >> C++ compilers. We could quibble about the value of "ancient" of >> course --- Peter, do you have an idea when this construct became >> widely supported? >> >> I do think it might be a better idea to put a #error there instead >> of silently disabling static assertions. Then at least we could >> hope to get complaints if anyone *is* trying to use ancient C++, >> and thereby gauge whether it's worth working any harder for this. > > I guess my question was whether we couldn't just use the same > workaround we use for old C compilers. This got unanswered and the thread has stalled for two months, so for now I am marking the patch as returned with feedback. -- Michael
On 11/29/17 00:35, Michael Paquier wrote: > On Fri, Sep 1, 2017 at 8:28 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Thu, Aug 31, 2017 at 6:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Meh. We support ancient versions of C for backwards compatibility >>> reasons, but considering that compiling backend code with C++ isn't >>> officially supported at all, I'm not sure we need to cater to ancient >>> C++ compilers. We could quibble about the value of "ancient" of >>> course --- Peter, do you have an idea when this construct became >>> widely supported? >>> >>> I do think it might be a better idea to put a #error there instead >>> of silently disabling static assertions. Then at least we could >>> hope to get complaints if anyone *is* trying to use ancient C++, >>> and thereby gauge whether it's worth working any harder for this. >> >> I guess my question was whether we couldn't just use the same >> workaround we use for old C compilers. > > This got unanswered and the thread has stalled for two months, so for > now I am marking the patch as returned with feedback. The answer to that question is "because it doesn't work". I'd still like a review of this patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 29, 2017 at 9:26 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > I'd still like a review of this patch. I don't think there's much to review apart from this one issue. Neither Tom nor I seem to be convinced about: +/* not worth providing a workaround */ I suggested that it was worth providing a workaround, and Tom suggested that the case might be so rare that we could just #error if happens. If you agree that it's never likely to come up, I suggest going with Tom's #error proposal; otherwise, I suggest trying to find a workable workaround. Apart from that, the only thing I see is that it seems like the comment block just before your code changes might need some updating. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Nov 29, 2017 at 9:26 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> I'd still like a review of this patch.
> I don't think there's much to review apart from this one issue.
> Neither Tom nor I seem to be convinced about:
> +/* not worth providing a workaround */
> I suggested that it was worth providing a workaround, and Tom
> suggested that the case might be so rare that we could just #error if
> happens. If you agree that it's never likely to come up, I suggest
> going with Tom's #error proposal; otherwise, I suggest trying to find
> a workable workaround.
Right now, we have the property that every build enforces static
assertions, albeit with variable quality of the error messages.
I strongly disagree that it's okay to throw that property away.
I do think that we could put an #error here instead, and wait to see
if anyone complains before expending effort on a workaround.
> Apart from that, the only thing I see is that it seems like the
> comment block just before your code changes might need some updating.
Agreed, that would need some love as well. I have no other comments
either.
regards, tom lane
On 2017-11-29 09:41:15 -0500, Robert Haas wrote: > On Wed, Nov 29, 2017 at 9:26 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: > > I'd still like a review of this patch. > > I don't think there's much to review apart from this one issue. > Neither Tom nor I seem to be convinced about: > > +/* not worth providing a workaround */ FWIW, I think that's a perfectly reasonable choice. Adding complications in making static assertions work for random archaic compilers when compiling with c++ just doesn't seem worth more than a few mins of thought. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2017-11-29 09:41:15 -0500, Robert Haas wrote:
>> +/* not worth providing a workaround */
> FWIW, I think that's a perfectly reasonable choice. Adding complications
> in making static assertions work for random archaic compilers when
> compiling with c++ just doesn't seem worth more than a few mins of
> thought.
I don't think anyone is advocating that we need to develop a solution
that works, at least not pending somebody actually complaining that
they want to build PG with an ancient C++ compiler. I just want
"we don't support this" to be spelled "#error", rather than dumping off
a load of reasoning about what might happen without functioning static
asserts --- on a weird compiler, no less --- onto our future selves.
regards, tom lane
On 2017-11-29 16:39:14 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2017-11-29 09:41:15 -0500, Robert Haas wrote: > >> +/* not worth providing a workaround */ > > > FWIW, I think that's a perfectly reasonable choice. Adding complications > > in making static assertions work for random archaic compilers when > > compiling with c++ just doesn't seem worth more than a few mins of > > thought. > > I don't think anyone is advocating that we need to develop a solution > that works, at least not pending somebody actually complaining that > they want to build PG with an ancient C++ compiler. I just want > "we don't support this" to be spelled "#error", rather than dumping off > a load of reasoning about what might happen without functioning static > asserts --- on a weird compiler, no less --- onto our future selves. C++ static asserts are somewhat new (C++11), so I'm unconvinced by that. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2017-11-29 16:39:14 -0500, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> FWIW, I think that's a perfectly reasonable choice. Adding complications
>>> in making static assertions work for random archaic compilers when
>>> compiling with c++ just doesn't seem worth more than a few mins of
>>> thought.
>> I don't think anyone is advocating that we need to develop a solution
>> that works, at least not pending somebody actually complaining that
>> they want to build PG with an ancient C++ compiler. I just want
>> "we don't support this" to be spelled "#error", rather than dumping off
>> a load of reasoning about what might happen without functioning static
>> asserts --- on a weird compiler, no less --- onto our future selves.
> C++ static asserts are somewhat new (C++11), so I'm unconvinced by that.
Wait a minute --- you were just saying that only archaic C++ compilers
were at issue. You don't get to have that both ways.
regards, tom lane
On 11/29/17 10:03, Tom Lane wrote: > Right now, we have the property that every build enforces static > assertions, albeit with variable quality of the error messages. > I strongly disagree that it's okay to throw that property away. > I do think that we could put an #error here instead, and wait to see > if anyone complains before expending effort on a workaround. I guess the question is whether we would rather be able to have users continue to use older C++ compilers, or be super picky about static assertions. In the g++ line, the oldest compiler that supports static assertions is g++-6, and g++-5 doesn't support it. I think that is recent enough to be a concern. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 11/29/17 10:03, Tom Lane wrote:
>> Right now, we have the property that every build enforces static
>> assertions, albeit with variable quality of the error messages.
>> I strongly disagree that it's okay to throw that property away.
>> I do think that we could put an #error here instead, and wait to see
>> if anyone complains before expending effort on a workaround.
> I guess the question is whether we would rather be able to have users
> continue to use older C++ compilers, or be super picky about static
> assertions.
Uh, what is this "continue to use" bit? We've never supported
building the backend with C++ compilers. Nor, since the whole
point here is that StaticAssert doesn't work with C++, is there
any existing tradition of building extensions that use StaticAssert
with C++. So I think it's highly likely that if we put in
an #error there, there will be exactly zero complaints.
I'd much rather continue to be sure that StaticAssert does what it
says on the tin than retroactively improve the compatibility situation
for older compilers.
(BTW, why is it that we can't fall back on the negative-width-bitfield
trick for old g++?)
regards, tom lane
On 12/11/17 16:45, Tom Lane wrote: >> I guess the question is whether we would rather be able to have users >> continue to use older C++ compilers, or be super picky about static >> assertions. > > Uh, what is this "continue to use" bit? We've never supported > building the backend with C++ compilers. The problem is static assertions in inline functions in header files. We do support using the header files in C++ extensions. But now there is a problem that if a C++ extension happens to pull in a header file that has a static assertion, it doesn't compile anymore, but it used to before the static assertion was introduced. (Currently, we have very few static assertions in header files, so the problem is not relevant in practice yet, but the use of both static assertions and inline functions is clearly expanding.) > (BTW, why is it that we can't fall back on the negative-width-bitfield > trick for old g++?) The complaint is error: types may not be defined in 'sizeof' expressions -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 12/11/17 16:45, Tom Lane wrote:
>> (BTW, why is it that we can't fall back on the negative-width-bitfield
>> trick for old g++?)
> The complaint is
> error: types may not be defined in 'sizeof' expressions
Hmm, well, surely there's more than one way to do that; the sizeof
is just a convenient way to wrap it in C. Wouldn't a typedef serve
just as well?
(Googling the topic shows that this wheel has been invented
before, BTW.)
regards, tom lane
On 12/11/17 17:12, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 12/11/17 16:45, Tom Lane wrote: >>> (BTW, why is it that we can't fall back on the negative-width-bitfield >>> trick for old g++?) > >> The complaint is >> error: types may not be defined in 'sizeof' expressions > > Hmm, well, surely there's more than one way to do that; the sizeof > is just a convenient way to wrap it in C. Wouldn't a typedef serve > just as well? Here is another attempt, which has the desired effect with the handful of compilers I have available. (With the recent changes to AllocSetContextCreate() using a static assertion, the current state now breaks actual extensions written in C++ in some configurations, so this has become a bit of a must-fix for PG11.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 12/11/17 17:12, Tom Lane wrote:
>> Hmm, well, surely there's more than one way to do that; the sizeof
>> is just a convenient way to wrap it in C. Wouldn't a typedef serve
>> just as well?
> Here is another attempt, which has the desired effect with the handful
> of compilers I have available.
I can confirm that the negative-bitfield-width method employed here
has the desired effects (i.e., error or not, without unwanted warnings)
on the oldest C++ compilers I have handy, namely
$ g++ -v
Reading specs from /usr/local/lib/gcc-lib/hppa2.0-hp-hpux10.20/2.95.3/specs
gcc version 2.95.3 20010315 (release)
$ g++ -v
Using built-in specs.
Target: powerpc-apple-darwin8
Configured with: /private/var/tmp/gcc/gcc-5341.obj~1/src/configure --disable-checking -enable-werror --prefix=/usr
--mandir=/share/man--enable-languages=c,objc,c++,obj-c++ --program-transform-name=/^[cg][^.-]*$/s/$/-4.0/
--with-gxx-include-dir=/include/c++/4.0.0--with-slibdir=/usr/lib --build=powerpc-apple-darwin8
--host=powerpc-apple-darwin8--target=powerpc-apple-darwin8
Thread model: posix
gcc version 4.0.1 (Apple Computer, Inc. build 5341)
I do not have a well-informed opinion on whether
#if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
is an appropriate test for static_assert() being available, but I'm
pretty suspicious of it because none of my C++ compilers seem to
take that path, not even recent stuff like clang 9.0.0. However,
since the negative-bitfield-width code path works anyway, that's
something we could refine later.
In short, I think this could be committed as-is, but later we might want
to do some more research on how to tell whether static_assert() is
available.
regards, tom lane
On 12/20/17 00:57, Tom Lane wrote: > I do not have a well-informed opinion on whether > > #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410 > > is an appropriate test for static_assert() being available, but I'm > pretty suspicious of it because none of my C++ compilers seem to > take that path, not even recent stuff like clang 9.0.0. For clang, you apparently need to pass -std=c++11 or higher. With g++ >=6, that's the default. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 12/20/17 00:57, Tom Lane wrote:
>> I do not have a well-informed opinion on whether
>> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
>> is an appropriate test for static_assert() being available, but I'm
>> pretty suspicious of it because none of my C++ compilers seem to
>> take that path, not even recent stuff like clang 9.0.0.
> For clang, you apparently need to pass -std=c++11 or higher. With g++
> >=6, that's the default.
Ah. I did try -std=c++0x on one machine, but forgot to do so with
the newer clang. That does seem to make it take the static_assert
path on recent macOS.
regards, tom lane
On 12/20/17 10:51, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> On 12/20/17 00:57, Tom Lane wrote: >>> I do not have a well-informed opinion on whether >>> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410 >>> is an appropriate test for static_assert() being available, but I'm >>> pretty suspicious of it because none of my C++ compilers seem to >>> take that path, not even recent stuff like clang 9.0.0. > >> For clang, you apparently need to pass -std=c++11 or higher. With g++ >>> =6, that's the default. > > Ah. I did try -std=c++0x on one machine, but forgot to do so with > the newer clang. That does seem to make it take the static_assert > path on recent macOS. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services