RFC: add DLLEXPORT to utf8proc_get_property

[tkelman]

ref JuliaLang/julia#7761 (comment)

This is needed to finish building the system image when using MSVC to build Julia. More of the API will probably need to be DLLEXPORT'ed to pass any Julia tests that use other functions.

[tkelman]

whoops, thought it wasn't working any more after that second commit, it was just submodules getting the better of me for a minute.

[jiahao]

It looks fine to me, but this isn't my area.

[stevengj]

You have this under #ifdef _MSC_VER (MS Visual C++), but it seems like this should be in a separate #ifdef _WIN32 clause (so that it is used for any compiler on Windows).

[tkelman]

Perhaps it should. I wanted to avoid changing the existing MinGW build too dramatically, where either we have a -Wl,--export-all-symbols somewhere or MinGW is just defaulting to exporting everything if there are no explicit dllexport instructions provided. If we make this definition apply for MinGW as well, then any function other than these 4 (which look to be the only ones currently ccalled somewhere in base Julia) will no longer be exported from the DLL (libjulia in this case, since we statically link in libmojibake).

In some ways the __declspec(dllexport) annotation is like saying "this is part of the public API if this library gets linked into a DLL." I don't know enough about utf8proc to draw that line beyond the 4 functions that are used in base Julia code (not sure about anything in src/ for example) - should everything be exported?

[stevengj]

Every function in the header file should be exported.

[tkelman]

@stevengj okeydokey, thanks. I'll work on that (and change the DLLEXPORT define to be for any _WIN32, and test with MinGW to make sure that doesn't break anything) in about half an hour, after the F1 race is over.

[tkelman]

Every function in the header is now exported. I left off the constant int8 array utf8proc_utf8class, don't know whether those values are considered part of the API.

Do we want to #define DLLEXPORT __attribute__ ((visibility("default"))) on non-Windows, as is done in Julia?

[stevengj]

__attribute__ ((visibility("default"))) is a gcc-specific thing (although it is probably also supported in icc), so we would only want that if __GNUC__ (IIRC) is defined.

Actually, it looks like it should only be for __GNUC__ >= 4.

[stevengj]

In general, I would think that anything listed in the header file should be in the public API. (Conversely, if it isn't in the public API, it shouldn't be in the header file.) Including this global array (which tells you the length of a codepoint's encoding from the first byte, and hence is useful for inlined iteration over a UTF-8 string).

[tkelman]

I'll add DLLEXPORT on the global array then.

Hrm, Julia uses the __attribute__ for all non-Windows cases here https://github.com/JuliaLang/julia/blob/f7172d3968cfab44d6026fdcadc2ace32e6aef1e/src/support/dtypes.h#L66, but utf8proc is probably a bit more portable to obscure systems and decade-old GCC3 compilers than Julia would be.

[stevengj]

Yes, the more basic the library, the more portable I feel it should be, within reason.

[tkelman]

Makes perfect sense to me. I don't get quite what __attribute__ ((visibility("default"))) does exactly, so I will defer to your judgement for whether or not we want it here.

[stevengj]

It would probably be better to use something like _MOJIBAKE_EXPORTS here to avoid accidentally clashing with user-defined macros. (It is not too far-fetched to imagine mojibake.h being #included in a project that defines a LIBRARY_EXPORTS macro.)

[tkelman]

@stevengj sure. Did you mean with a leading underscore?

[stevengj]

Yes, I put the leading underscore there just to emphasize that this is not a macro for users to fiddle with.

[tkelman]

Depends what you mean by "users," but anyone who wants to depend on this library and have the API exported from a DLL will need to be aware of and define this macro (at mojibake build time), particularly if incorporating the library as a dependency in a build system that doesn't use the Makefile here.

[stevengj]

Looks good to merge, in any case.

[tkelman]

Great, thanks for looking it over. Can continue to be refined, of course.