user build broken: U_SHOW_CPLUSPLUS_API used before including ICU headers

Description

We have broken builds with ICU 65rc due to this in icu4c/source/i18n/unicode/plurrule.h (didn't check other files):

which I believe is from [ICU-20530].

If the user does not include another ICU header before this, then U_SHOW_CPLUSPLUS_API is not yet defined, and the headers appears empty. Not even the icu namespace gets defined.

Please swap the order:

Essentially, we need to be careful to always include utypes.h before testing any configuration macros.

Please also check files other than plurrule.h.

This could be testable in the header test by adding something to the test .cpp files that requires the icu namespace to be defined. Maybe forward-declare a class inside the namespace?

Activity

Show:
Jeff Genovy
September 27, 2019, 1:41 AM
Jeff Genovy
September 26, 2019, 11:56 PM

Follow up ticket:

Jeff Genovy
September 26, 2019, 11:52 PM

Sorry for the delay, I got pulled off on to other things.

I would be somewhat inclined to agree that for 65 we maybe just fix this one issue in the header plurrule.h and then I will file a follow-up ticket for 66 to add an automated test.

I will create a PR in a moment to fix the header.

Markus Scherer
September 26, 2019, 10:10 PM

We could just fix the known/confirmed failure now (plurrule.h) and do the automated test later… And maybe this is such an unusual outlier that we need not have an automated test? (Although easy to make the same mistake again in a new header file.)

Markus Scherer
September 26, 2019, 9:52 PM

Thanks for the detailed analysis!

The files to test should be those that contain “{{#if U_SHOW_CPLUSPLUS_API}}“ which I assume automatically excludes the headers that don’t work. Is it possible in the test script to grep and use the list of files?

Fixed
Your pinned fields
Click on the next to a field label to start pinning.

Assignee

Jeff Genovy

Reporter

Markus Scherer

Components

Priority

major

Time Needed

Minutes

Fix versions