Thread: cpluspluscheck vs ICU
Hi,
I was about to propose adding headerscheck / cpluspluscheck to the CI file so
that cfbot can catch future issues. Unfortunately running cpluspluscheck with
ICU enabled is, um, not fun: There's 30k lines of error output.
/home/andres/src/postgresql/src/tools/pginclude/cpluspluscheck /home/andres/src/postgresql
/home/andres/build/postgres/dev-assert/vpath
In file included from /usr/include/c++/12/bits/stl_algobase.h:60,
from /usr/include/c++/12/memory:63,
from /usr/include/unicode/localpointer.h:45,
from /usr/include/unicode/unorm2.h:34,
from /usr/include/unicode/unorm.h:25,
from /usr/include/unicode/ucol.h:17,
from /home/andres/src/postgresql/src/include/utils/pg_locale.h:19,
from /home/andres/src/postgresql/src/include/tsearch/ts_locale.h:20,
from /tmp/cpluspluscheck.H59Y6V/test.cpp:3:
/usr/include/c++/12/bits/functexcept.h:101:3: error: conflicting declaration of C function ‘void
std::__throw_ios_failure(constchar*, int)’
101 | __throw_ios_failure(const char*, int) __attribute__((__noreturn__));
| ^~~~~~~~~~~~~~~~~~~
/usr/include/c++/12/bits/functexcept.h:98:3: note: previous declaration ‘void std::__throw_ios_failure(const char*)’
98 | __throw_ios_failure(const char*) __attribute__((__noreturn__));
| ^~~~~~~~~~~~~~~~~~~
In file included from /usr/include/c++/12/bits/stl_algobase.h:63:
/usr/include/c++/12/ext/numeric_traits.h:50:3: error: template with C linkage
50 | template<typename _Tp>
| ^~~~~~~~
/tmp/cpluspluscheck.H59Y6V/test.cpp:1:1: note: ‘extern "C"’ linkage started here
1 | extern "C" {
| ^~~~~~~~~~
...
with one warning for each declaration in numeric_traits.h, I think.
So, there's two questions:
1) How can we prevent this problem when ICU support is enabled?
2) Can we prevent such absurdly long error output?
For 2), perhaps we should just specify EXTRAFLAGS=-fmax-errors=10 in the
cpluspluscheck invocation, or add it in cpluspluscheck itself?
For 1), I don't immediately see a minimal solution other than ignoring it in
cpluspluscheck, similar to pg_trace.h/probes.h.
A different / complimentary approach could be to add -Wc++-compat to the
headerscheck invocation. Both gcc and clang understand that.
But neither of these really gets to the heart of the problem. There's still no
way for C++ code to include pg_locale.h correctly. And in contrast to
pg_trace.h/probes.h pg_locale.h is somewhat important.
This isn't a new problem, afaics.
Perhaps we should strive to remove the use of ICU headers from within our
headers? The members of pg_locale are just pointers and could thus be void *,
and HAVE_UCOL_STRCOLLUTF8 could be computed at configure time or such.
Greetings,
Andres Freund
Hi, On 2022-03-22 17:20:24 -0700, Andres Freund wrote: > I was about to propose adding headerscheck / cpluspluscheck to the CI file so > that cfbot can catch future issues. The attached patch does so, with ICU disabled to avoid the problems discussed in the thread. Example run: https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0 Unless somebody sees a reason not to, I'm planning to commit this soon. Greetings, Andres Freund
Attachment
On Wed, Mar 23, 2022 at 3:23 PM Andres Freund <andres@anarazel.de> wrote: > On 2022-03-22 17:20:24 -0700, Andres Freund wrote: > > I was about to propose adding headerscheck / cpluspluscheck to the CI file so > > that cfbot can catch future issues. > > The attached patch does so, with ICU disabled to avoid the problems discussed > in the thread. Example run: > https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0 > > Unless somebody sees a reason not to, I'm planning to commit this soon. LGTM.
On 3/22/22 22:23, Andres Freund wrote: > Hi, > > On 2022-03-22 17:20:24 -0700, Andres Freund wrote: >> I was about to propose adding headerscheck / cpluspluscheck to the CI file so >> that cfbot can catch future issues. > The attached patch does so, with ICU disabled to avoid the problems discussed > in the thread. Example run: > https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0 > > Unless somebody sees a reason not to, I'm planning to commit this soon. > That only helps when running the CI/cfbot setup. Fixing it for other (manual or buildfarm) users would be nice. Luckily crake isn't building with ICU. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Hi, On 2022-03-23 08:19:38 -0400, Andrew Dunstan wrote: > On 3/22/22 22:23, Andres Freund wrote: > > On 2022-03-22 17:20:24 -0700, Andres Freund wrote: > >> I was about to propose adding headerscheck / cpluspluscheck to the CI file so > >> that cfbot can catch future issues. > > The attached patch does so, with ICU disabled to avoid the problems discussed > > in the thread. Example run: > > https://cirrus-ci.com/task/6326161696358400?logs=headers_headerscheck#L0 > > > > Unless somebody sees a reason not to, I'm planning to commit this soon. > > > > That only helps when running the CI/cfbot setup. Fixing it for other > (manual or buildfarm) users would be nice. Luckily crake isn't building > with ICU. Oh, I agree we need to fix it properly. I just don't yet know how to - see the list of alternatives upthread. Seems no reason to hold up preventing further problems via CI / cfbot though. Greetings, Andres Freund
Hi,
On 2022-03-23 08:56:17 -0700, Andres Freund wrote:
> On 2022-03-23 08:19:38 -0400, Andrew Dunstan wrote:
> > On 3/22/22 22:23, Andres Freund wrote:
> > That only helps when running the CI/cfbot setup. Fixing it for other
> > (manual or buildfarm) users would be nice. Luckily crake isn't building
> > with ICU.
>
> Oh, I agree we need to fix it properly. I just don't yet know how to - see the
> list of alternatives upthread. Seems no reason to hold up preventing further
> problems via CI / cfbot though.
I just hit this once more - and I figured out a fairly easy fix:
We just need a
#ifndef U_DEFAULT_SHOW_DRAFT
#define U_DEFAULT_SHOW_DRAFT 0
#endif
before including unicode/ucol.h.
At first I was looking at
#define U_SHOW_CPLUSPLUS_API 0
and
#define U_HIDE_INTERNAL_API 1
which both work, but they are documented to be internal.
The reason for the #ifndef is that pg_locale.h might be included by .c files
that already included ICU headers, which then otherwise would cause macro
redefinition warnings. E.g. in formatting.c.
Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid
that issue.
The only other thing I see is to do something like:
#ifdef USE_ICU
#ifdef __cplusplus
/* close extern "C", otherwise we'll get errors from within ICU */
}
#endif /* __cplusplus */
#include <unicode/ucol.h>
#ifdef __cplusplus
extern "C" {
#endif /* __cplusplus */
#endif /* USE_ICU */
which seems mighty ugly.
Regards,
Andres
Hi,
On 2023-03-10 19:37:27 -0800, Andres Freund wrote:
> On 2022-03-23 08:56:17 -0700, Andres Freund wrote:
> > On 2022-03-23 08:19:38 -0400, Andrew Dunstan wrote:
> > > On 3/22/22 22:23, Andres Freund wrote:
> > > That only helps when running the CI/cfbot setup. Fixing it for other
> > > (manual or buildfarm) users would be nice. Luckily crake isn't building
> > > with ICU.
> >
> > Oh, I agree we need to fix it properly. I just don't yet know how to - see the
> > list of alternatives upthread. Seems no reason to hold up preventing further
> > problems via CI / cfbot though.
>
> I just hit this once more - and I figured out a fairly easy fix:
>
> We just need a
> #ifndef U_DEFAULT_SHOW_DRAFT
> #define U_DEFAULT_SHOW_DRAFT 0
> #endif
> before including unicode/ucol.h.
>
> At first I was looking at
> #define U_SHOW_CPLUSPLUS_API 0
> and
> #define U_HIDE_INTERNAL_API 1
> which both work, but they are documented to be internal.
Err. Unfortunately only the U_SHOW_CPLUSPLUS_API approach actually works. The
others don't, not quite sure what I was doing earlier.
So it's either relying on a define marked as internal, or the below:
> Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid
> that issue.
>
>
> The only other thing I see is to do something like:
>
> #ifdef USE_ICU
> #ifdef __cplusplus
> /* close extern "C", otherwise we'll get errors from within ICU */
> }
> #endif /* __cplusplus */
>
> #include <unicode/ucol.h>
>
> #ifdef __cplusplus
> extern "C" {
> #endif /* __cplusplus */
>
> #endif /* USE_ICU */
>
> which seems mighty ugly.
Greetings,
Andres Freund
Hi, On 2023-03-10 20:10:30 -0800, Andres Freund wrote: > On 2023-03-10 19:37:27 -0800, Andres Freund wrote: > > I just hit this once more - and I figured out a fairly easy fix: > > > > We just need a > > #ifndef U_DEFAULT_SHOW_DRAFT > > #define U_DEFAULT_SHOW_DRAFT 0 > > #endif > > before including unicode/ucol.h. > > > > At first I was looking at > > #define U_SHOW_CPLUSPLUS_API 0 > > and > > #define U_HIDE_INTERNAL_API 1 > > which both work, but they are documented to be internal. > > Err. Unfortunately only the U_SHOW_CPLUSPLUS_API approach actually works. The > others don't, not quite sure what I was doing earlier. > > So it's either relying on a define marked as internal, or the below: > > > Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid > > that issue. > > > > > > The only other thing I see is to do something like: > > [ugly] > > which seems mighty ugly. The ICU docs talk about it like it's not really internal: https://github.com/unicode-org/icu/blob/720e5741ccaa112c4faafffdedeb7459b66c5673/docs/processes/release/tasks/healthy-code.md#test-icu4c-headers So I'm inclined to go with that solution. Any comments? Arguments against? Greetings, Andres Freund
On 08.08.23 01:35, Andres Freund wrote: > Hi, > > On 2023-03-10 20:10:30 -0800, Andres Freund wrote: >> On 2023-03-10 19:37:27 -0800, Andres Freund wrote: >>> I just hit this once more - and I figured out a fairly easy fix: >>> >>> We just need a >>> #ifndef U_DEFAULT_SHOW_DRAFT >>> #define U_DEFAULT_SHOW_DRAFT 0 >>> #endif >>> before including unicode/ucol.h. >>> >>> At first I was looking at >>> #define U_SHOW_CPLUSPLUS_API 0 >>> and >>> #define U_HIDE_INTERNAL_API 1 >>> which both work, but they are documented to be internal. >> >> Err. Unfortunately only the U_SHOW_CPLUSPLUS_API approach actually works. The >> others don't, not quite sure what I was doing earlier. >> >> So it's either relying on a define marked as internal, or the below: >> >>> Alternatively we could emit U_DEFAULT_SHOW_DRAFT 0 into pg_config.h to avoid >>> that issue. >>> >>> >>> The only other thing I see is to do something like: >>> [ugly] >>> which seems mighty ugly. > > The ICU docs talk about it like it's not really internal: > https://github.com/unicode-org/icu/blob/720e5741ccaa112c4faafffdedeb7459b66c5673/docs/processes/release/tasks/healthy-code.md#test-icu4c-headers > > So I'm inclined to go with that solution. This looks sensible to me. Perhaps undef U_SHOW_CPLUSPLUS_API after including the headers, so that if extensions want to use the ICU C++ APIs, they are not tripped up by this?