ICU-23136 Remove __declspec(dllexport) from class Locale on Windows

[roubert]

Checklist

• [X] Required: Issue filed: ICU-23136
• [X] Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
• [X] Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
• [x] Issue accepted (done by Technical Committee after discussion)
• [ ] Tests included, if applicable
• [ ] API docs and/or User Guide docs changed or added, if applicable

[roubert]

There's a testtagsguards.sh failure in clang-release-build-and-test, what's the correct solution for that?

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

Notice: the branch changed across the force-push!

• icu4c/source/common/unicode/utypes.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[mihnita]

Digging in the export tables, and a bunch of entries seems to be dropped (not exported anymore).

I was unable to figure out a clear rule. I've seen both static an non static methods disappearing, public / protected / private.

Most worrisome (I think) is the disappearance of some public entries, mostly constructors. And not only the default ones, generated by the compiler, but constructors with parameters, that I can find declared in the header file.

The intriguing part is that not all constructors disappear, only some. Again, I can't see the rule.

[mihnita]

Why are you replacing U_EXPORT2 after the return type with U_EXPORT before the return type?

That I do know! That's necessary to do, for otherwise MSVC will fail with a compilation error.

I think that since U_EXPORT ends up being __declspec(dllexport), this applies:

To export functions, the __declspec(dllexport) keyword must appear to the left of the calling-convention keyword, if a keyword is specified. For example:

__declspec(dllexport) void __cdecl Function1(void);

To export all of the public data members and member functions in a class, the keyword must appear to the left of the class name as follows:

class __declspec(dllexport) CExampleExport : public CObject
{ ... class definition ... };

https://learn.microsoft.com/en-us/cpp/build/exporting-from-a-dll-using-declspec-dllexport?view=msvc-170

So U_EXPORT must be on the left of the return type.

---

U_EXPORT2 maps to __cdecl

Place the __cdecl modifier before a variable or a function name.

https://learn.microsoft.com/en-us/cpp/cpp/cdecl?view=msvc-170

I think that calling it ?_EXPORT? is confusing, as these are not actually exported. But hey, it is probably ancient history, not worth shaking the boat.

[markusicu]

Digging in the export tables, and a bunch of entries seems to be dropped (not exported anymore). [...] I've seen both static an non static methods disappearing, public / protected / private. Most worrisome (I think) is the disappearance of some public entries, mostly constructors. And not only the default ones, generated by the compiler, but constructors with parameters, that I can find declared in the header file.

C++ likes to auto-generate member functions. We have tried to define them explicitly, so that we can tag them with their status, but I think that's becoming less tenable with it generating more and more things like symmetric operator== and opterator!=.

If the auto-generated ones are no longer reliably exported, then that seems like a problem.

No longer reliably exporting explicitly declared methods is of course even worse :-(

Why are you replacing U_EXPORT2 after the return type with U_EXPORT before the return type?

That I do know! That's necessary to do, for otherwise MSVC will fail with a compilation error.

I think that since U_EXPORT ends up being __declspec(dllexport), this applies:

I reminded myself that U_COMMON_API is the same as U_EXPORT or U_IMPORT or nothing depending on whether we compile ICU itself or user code.

So I think these static member functions should use U_COMMON_API instead of U_EXPORT.

And if it is harmless (= does not break CI) to also keep the U_EXPORT2 where it's always been, then so much the better.

IOW please try to treat the static member functions the same as the non-static ones.

@mihnita since you have a Windows machine, can you please try this? For example:

U_COMMON_API static const Locale& U_EXPORT2 getDefault();

[mihnita]

@mihnita since you have a Windows machine, can you please try this? For example: U_COMMON_API static const Locale& U_EXPORT2 getDefault();

There is no difference in the export table between this and what Fredrik does.

[mihnita]

@mihnita since you have a Windows machine, can you please try this? For example:

I tried, there is no difference in the export table between this and the result with the current PR change.

And I am pretty sure I've commented this a few days ago, but somehow GitHub "forgot" it. Or maybe my memory is playing tricks on me...

[markusicu]

@mihnita since you have a Windows machine, can you please try this? For example:

I tried, there is no difference in the export table between this and the result with the current PR change.

Ok. So Windows/Mac/Linux don't care either way.

The current attribute combinations are a result of long-ago trial-and-error with a larger set of platforms. Is there any reason not to make the more conservative change? If not, then I am willing to add a commit to that effect.

[roubert]

Is there any reason not to make the more conservative change?

I would just like to avoid making any change at all before we find someone who can say what the preferred way of doing this really is. At the moment we have two potential options but we don't know whether any of them is the preferred way, there might be a third option that we're not yet aware of, so making any such change now would be wasted effort if it later needs to be changed again. That's all.

[markusicu]

Is there any reason not to make the more conservative change?

I would just like to avoid making any change at all before we find someone who can say what the preferred way of doing this really is.

Well, we don't seem to have an actual expert. You are making changes, and it scares me a bit that they seem to do more than what we need.

Note also that the macros like U_COMMON_API resolve to either import or export depending on which code you are compiling. U_COMMON_API wants to export from the common library, and import into the i18n library, into ICU tools & tests, and into non-ICU code. Changing U_COMMON_API to just U_EXPORT seems misleading at best.

I will give it a try, see what CI says, and ask Mihai to try it on his Windows machine as well.

[mihnita]

There is a pretty long list of differences between what is exported "the old way" and "the new way"

There are differences even for members declared public, which I would expect to be preserved. And there are still present a lot of members that are private or protected. Which I would expect to stop being exported.

Let's say we don't worry too much about extra exports (although I kind of think was the point of this exercise?)

But missing public exports seems worrisome.

See here an archive with the old / new exports: https://mihai-nita.net/tmp/diff_exports.zip

---

There is no risk that I built at different commit points. Because I went to head, built / dump / cleanup, then applied the PR as a patch, then built / dump. So the only differences between the 2 dumps was the PR.

[roubert]

There are differences even for members declared public, which I would expect to be preserved.

Apart from the ones that I believe to have been exported by mistake before, have you seen any other public declarations missing now? I can't find any such in your dump files (but I haven't checked every single entry that differs).

And there are still present a lot of members that are private or protected.

Are any of those such members that the changes in this PR ought to remove? The changes in this PR only touch a tiny fraction of the code base, most private or protected members should be unaffected by these changes.

[markusicu]

There is a pretty long list of differences between what is exported "the old way" and "the new way"

Mihai and I are looking at the diffs. Some of the symbols that are no longer supported are private functions, and as long as they are not called from inline code in the header file, that's fine.

I worry a bit about no longer exporting some of the vtables, for example

const icu_78::Locale::`vftable'
const icu_78::DateFormatSymbols::`vftable'
const icu_78::DateIntervalFormat::`vftable'

and a bunch of others. Are they not needed?

On the other hand, if CI checks are happy... :shrug: ?

[markusicu]

Added another commit after Mihai and I reviewed the changes in symbol exports.

If this passes CI

I broke CI. I should have known better: The three internal classes that I exported again have Locale fields, which must be why Fredrik un-exported them. I changed these to U_I18N_API_CLASS class now.

[markusicu]

It looks like the MSVC builds, and thus CI checks overall, are happy now. So we are back to my suggestion:

If this passes CI and no one has ideas for what else to test and what more to change, then I suggest we remove the first commit (the temporary test one), squash the rest, approve, and merge.

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

Notice: the branch changed across the force-push!

• icu4c/source/common/locid.cpp is no longer changed in the branch
• icu4c/source/common/unicode/locid.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

If this passes CI and no one has ideas for what else to test and what more to change, then I suggest we remove the first commit (the temporary test one), squash the rest, approve, and merge.

Elsewhere, @mihnita gave me a "thumb up" for this. I have now removed the test commit 540a96af65a683793df3b667a3fe6e06055115c2 so this should be ready for review & approval. Once approved, let's squash, and then merge.

[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