ICU headers cannot be built within extern "C" scope

Description

For example:

Introduced by commit b7a3571b additional include of <memory> without guard.

Found by xmlsec build.

Reproduce:

Applied for:

  • char16ptr.h:#include <cstddef>

  • localpointer.h:#include <memory>

  • std_string.h:#include <string>

  • unistr.h:#include <cstddef>

In time the following requirement was not strictly followed:

Activity

Show:
Shane Carr
April 17, 2019, 6:06 PM

I also wanted to point out that there are sort of three cases for people including ICU headers:

1. C compiler only
2. C++ compiler, but want binary compatibility
3. C++ compiler, and don't care about binary compatibility

We provide header-only C++ APIs to help users in case 2. The problem with using a single U_HIDE_CPLUSPLUS_API is that it doesn't distinguish between cases 2 and 3. So it seems like maybe we want to add another #define to allow for all three configurations.

Jeff Genovy
April 17, 2019, 6:25 PM

Thank you for the comment Shane! That is a really good point.

I don’t think we’d necessarily want to change the existing macro, U_HIDE_CPLUSPLUS_API, that is used to guard “C++ helpers” in C headers, as there might be existing users that already are setting/defining the macro before including the header. (Even though ICU doesn't guarantee source compatibility, we should likely try to avoid breaking things if we can help it).

So perhaps another define might be used to guard the C++ headers that contain the C++ APIs? I'm not sure what the name of such a define would be though. I think you suggested something like U_SHOW_CPLUSPLUS_LIBRARY_API in the ICU-TC, but I don't have a strong opinion on the name.

We'll also want to add notes to the ICU 65 download page about migration if PR#613 is merged.

Existing users that use C++ headers inside "extern C" blocks would need to:

  • Move the header outside the extern block if they actually intended to use C++.

  • Define/Set the macro before including the header(s) if they didn’t intend to use C++.

Jeff Genovy
April 25, 2019, 10:02 PM
Edited

More notes based on the discussion in the ICU-TC call on 2019-04-24:

Supporting the second use case outline by Shane ( "C++ compiler, but want binary compat") while generally desirable, it is rather hard to do so in portable way. In order to do so cleanly, it might involve creating completely new headers and/or putting the "header-only" C++ parts into a separate namespace.

Additionally, it would be hard to create automated testing which could catch any sorts of accidental regressions in the CI builds. (Ex: somebody starting linking to a C++ export inside a "header-only" section of code).

Given this, the decision was to only guard the C++ headers behind the existing U_SHOW_CPLUSPLUS_API macro and to not create another macro for "header-only" C++ APIs for now.

That doesn't mean it will never happen, just that for the purpose of this ticket/issue, it would be out of scope.

The next PR would be to add the macro check to all the C++ headers.

I've started working on this in a branch here, but haven't created a PR yet: https://github.com/unicode-org/icu/compare/master...jefgen:jefgen/show-cpp-header-changes

Jeff Genovy
August 30, 2019, 5:47 PM

I added notes to the download page for ICU 65:

  • All of the public C++ headers now assume a default context of extern "C++".

  • All of the public C++ headers are guarded with the macro "U_SHOW_CPLUSPLUS_API". (See ).

There is now also a CI build-time check to ensure the second bullet above, so that new C++ headers don’t get added without the guard macro.

Thank you kindly Alon Bar-Lev for you help with this issue and with as well!

Shane Carr
September 5, 2019, 12:24 PM

Changing resolution to "Fixed" since there is a commit against this ticket.

Fixed

Assignee

Jeff Genovy

Reporter

Alon Bar-Lev

Components

Labels

None

Reviewer

None

Priority

assess

Time Needed

None

Fix versions

Configure