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

roubert · 2025-06-10T14:51:44Z

Checklist

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

icu4c/source/common/servloc.h

roubert · 2025-06-13T16:19:06Z

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

markusicu · 2025-06-13T17:27:18Z

icu4c/source/common/unicode/locid.h

+    struct Foo;
+    std::unique_ptr<Foo> foo;
+    std::variant<std::monostate, int, std::unique_ptr<Foo>> bar;


This looks like a leftover from experimenting. Please remove.

That's not a leftover, that's very intentionally there to verify that this all really works, just as the commit description says: "TEST: std::unique_ptr & std::variant members. [DO NOT MERGE]".

I wasn't looking one commit at a time. As long as you remember to remove this commit before merging it's ok.

markusicu · 2025-06-13T17:30:00Z

icu4c/source/common/unicode/locid.h

@@ -369,7 +372,7 @@ class U_COMMON_API Locale : public UObject {
     * @system
     * @stable ICU 2.0
     */
-    static const Locale& U_EXPORT2 getDefault();
+    U_EXPORT static const Locale& getDefault();


why are some methods U_EXPORT instead of U_COMMON_API?

I don't know, but it looks to me as if the pattern was that static members were U_EXPORT2 (while non-static members just inherited U_…_API from the class). Should it not be like that?

The whole song and dance with these macros has grown over a long period of time trying to make various platforms and compilers happy.

I assume that U_COMMON_API on a whole class also applies to static members, that's why I am surprised you are not adding it to them as well. I think we needed the additional U_EXPORT2 for some consistency in calling conventions.

I also don't know what effect it has that you are replacing U_EXPORT2 after the return type with U_EXPORT before the return type. These kinds of things are largely compiler-specific. I seem to remember that the mainframe compilers were especially picky, but we don't have them in CI. I guess we will see if anyone complains after ICU 78 that their platform is now broken, and then we ask them to come up with a pull request that fixes theirs and doesn't break others.

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

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.

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.

... but it has worked so far. Would it work with the U_EXPORT2 in its original place if the public static members also had U_COMMON_API like they (I assume) used to inherit from the enclosing class?

Would it work with the U_EXPORT2 in its original place if the public static members also had U_COMMON_API like they (I assume) used to inherit from the enclosing class?

I don't know.

Would it work with the U_EXPORT2 in its original place if the public static members also had U_COMMON_API like they (I assume) used to inherit from the enclosing class?

I don't know.

Please try this with one of the static functions and see if it passes CI.
I would be more comfortable if this was a more consistent move-U_COMMON_API-onto-every-public-member change.

Please try this with one of the static functions and see if it passes CI.

I don't have any reasonably easy way to try that. These changes are already done through trial-and-error until I found something that happened to work. Trying out more things just to see if they might work too wouldn't be a meaningful use of time.

icu4c/source/common/unicode/utypes.h

This makes private std::unique_ptr & std::variant members possible.

jira-pull-request-webhook · 2025-06-16T13:41:55Z

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 · 2025-06-18T08:36:26Z

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 · 2025-06-18T08:47:22Z

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 · 2025-06-18T17:52:55Z

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 · 2025-06-20T23:45:11Z

@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 · 2025-06-23T16:03:54Z

@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 · 2025-06-30T23:52:34Z

@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 · 2025-07-01T12:34:51Z

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.

roubert force-pushed the debug branch from 0df34d3 to 87fdb29 Compare June 11, 2025 12:23

unicode-org deleted a comment from jira-pull-request-webhook bot Jun 11, 2025

roubert closed this Jun 11, 2025

roubert deleted the debug branch June 11, 2025 12:48

roubert restored the debug branch June 11, 2025 12:49

roubert reopened this Jun 11, 2025

markusicu reviewed Jun 11, 2025

View reviewed changes

icu4c/source/common/servloc.h Outdated Show resolved Hide resolved

ICU-23136 TEST: std::unique_ptr & std::variant members. [DO NOT MERGE]

540a96a

roubert force-pushed the debug branch from 87fdb29 to 0366b19 Compare June 13, 2025 15:37

roubert changed the title ~~ICU-20392 DEBUG Windows DLL [DO NOT MERGE]~~ ICU-23136 Remove __declspec(dllexport) from class Locale on Windows Jun 13, 2025

roubert force-pushed the debug branch from 0366b19 to 4735a6a Compare June 13, 2025 15:43

unicode-org deleted a comment from jira-pull-request-webhook bot Jun 13, 2025

roubert force-pushed the debug branch from 4735a6a to 19b4922 Compare June 13, 2025 15:47

unicode-org deleted a comment from jira-pull-request-webhook bot Jun 13, 2025

roubert marked this pull request as ready for review June 13, 2025 16:19

markusicu reviewed Jun 13, 2025

View reviewed changes

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

7a54cb2

This makes private std::unique_ptr & std::variant members possible.

roubert force-pushed the debug branch from 19b4922 to 7a54cb2 Compare June 16, 2025 13:41

markusicu self-assigned this Jun 17, 2025

markusicu requested review from mihnita, eggrobin and richgillam June 17, 2025 18:27

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

roubert commented Jun 10, 2025 • edited by markusicu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

roubert commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jira-pull-request-webhook bot commented Jun 16, 2025

Uh oh!

mihnita commented Jun 18, 2025

Uh oh!

mihnita commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusicu commented Jun 18, 2025

Uh oh!

mihnita commented Jun 20, 2025

Uh oh!

mihnita commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusicu commented Jun 30, 2025

Uh oh!

roubert commented Jul 1, 2025

Uh oh!

Uh oh!

roubert commented Jun 10, 2025 •

edited by markusicu

Loading

mihnita commented Jun 18, 2025 •

edited

Loading

mihnita commented Jun 23, 2025 •

edited

Loading