ICU-9749 Move explicit instantiation definition, add declaration

[tkoeppe]

Checklist

• [x] Issue filed: https://unicode-org.atlassian.net/browse/ICU-9749
• [x] Updated PR title and link in previous line to include Issue number
• [x] Issue accepted
• [ ] Tests included
• [x] Documentation is changed or added

(Yes, I think so.)

[CLAassistant]

All committers have signed the CLA.

[jira-pull-request-webhook[bot]]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[jira-pull-request-webhook[bot]]

Notice: the branch changed across the force-push!

• icu4c/source/i18n/number_decimalquantity.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[jira-pull-request-webhook[bot]]

Notice: the branch changed across the force-push!

• icu4c/source/i18n/number_decimalquantity.cpp is different
• icu4c/source/i18n/number_decnum.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[jefgen]

It looks like the MSVC builds have the following warning with the change:

d:\a\1\s\icu4c\source\i18n\number_decnum.h(20): warning C4910: 'icu_64::MaybeStackHeaderAndArray<decNumber,char,34>': '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation (compiling source file number_decimalquantity.cpp) [D:\a\1\s\icu4c\source\i18n\i18n.vcxproj]

This should be an error not a warning in the CI builds. I will file a ticket to add this to the list of fatal warnings for the CI builds.

[jira-pull-request-webhook[bot]]

Notice: the branch changed across the force-push!

• icu4c/source/i18n/number_decnum.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[jefgen]

When I previous tried removing the dllexport on the MaybeStackHeaderAndArray I ran into errors like this:

1>g:\c\icu4c\source\i18n\number_decnum.h(62): error C4251: 'icu_64::number::impl::DecNum::fData': class 'icu_64::MaybeStackHeaderAndArray<decNumber,char,34>' needs to have dll-interface to be used by clients of class 'icu_64::number::impl::DecNum' (compiling source file number_utils.cpp)

I'm not an expert on how DLL exporting C++ classes work, but it seems like the error is essentially saying that (for MSVC) the MaybeStackHeaderAndArray class needs to exported since it is used by the DecNum class.

So I think we need to mark it for export (with the U_I18N_API macro), but then that doesn't work since apparently "'__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation" (C4910).

I think the incompatibility is because the extern in the header is deferring the instantiation to the .cpp file.

[tkoeppe]

It seems like we really want __declspec(import) (not export!) on the non-defining declaration. Is there a macro for that?

[jefgen]

It seems like we really want __declspec(import) (not export!) on the non-defining declaration. Is there a macro for that?

There is a U_IMPORT macro. Perhaps that would work?

(Aside: Thank you for your help with this @tkoeppe.)

[jira-pull-request-webhook[bot]]

Notice: the branch changed across the force-push!

• icu4c/source/i18n/number_decnum.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[tkoeppe]

My pleasure!

Trying U_IMPORT now, but I'm confused: by my reading of utypes.h, U_I18N_API should already be the same as U_IMPORT in number_decnum.h. How is it __declspec(extern) in the earlier warning?

[jefgen]

Hm, my reading of utypes.h is like this: In the file i18n\number_decnum.h the macro U_I18N_API is defined as U_EXPORT, since inside the i18n library the macro U_I18N_IMPLEMENTATION is defined. (Or maybe I am missing something?)

#elif defined(U_I18N_IMPLEMENTATION)
#define U_DATA_API     U_IMPORT
#define U_COMMON_API   U_IMPORT
#define U_I18N_API     U_EXPORT

[tkoeppe]

Who defines U_I18N_IMPLEMENTATION? Oh, does the build system add that?

[tkoeppe]

Hm, now we've been through all three options: the extern declaration...

• can't have nothing because we get an error that a __declspec needs to be present;
• can't be __declspec(import) because that's an "unresolved external symbol"; and
• can't be __declspec(export), because that triggers a warning.

Hmmm.

[jefgen]

Who defines U_I18N_IMPLEMENTATION? Oh, does the build system add that?

Right. For the MSVC builds it is defined in the .vcxproj file for the i18n library.

(See the line here: https://github.com/unicode-org/icu/blob/maint/maint-64/icu4c/source/i18n/i18n.vcxproj#L54 )

[jefgen]

At the moment, I'm not sure if it is possible to do this with the restrictions imposed by MSVC.

[jira-pull-request-webhook[bot]]

Notice: the branch changed across the force-push!

• icu4c/source/i18n/number_decimalquantity.cpp is different
• icu4c/source/i18n/number_decnum.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[tkoeppe]

Let's try this again. Three years ago I had problems getting this to work under MSVC, but maybe technology has caught yp now?