jll63 / yomm2

Fast, orthogonal, open multi-methods. Solve the Expression Problem in C++17.
Boost Software License 1.0
343 stars 18 forks source link

Support shared library builds #37

Closed iboB closed 1 year ago

iboB commented 1 year ago
jll63 commented 1 year ago

Thanks, this is very useful.

A few remarks...

I think that you did not build the test suite ;-) update_methods is a simple version of a more capable update_methods(catalog& cat, context& ctx), which lives in detail.hpp. Currently it is used only in tests, but at some point I will make it public. It allows separate collections of classes and methods.

I am a little reluctant to add a macro definition to core.hpp, because that header is supposed to please the hardcore macro haters. But I guess that this is a legitimate use, like include guards. I wonder, however, if YOMM2_API should be obfuscated, like the helper macros here.

I don't think that BUILDING_YOMM2 is needed. When compiling with VERBOSE=1 I noticed:

cd /home/jll/dev/yomm2/build/src && /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBUILDING_YOMM2=1 -DYOMM2_SHARED=1 -Dyomm2_EXPORTS -I/home/jll/dev/yomm2/include -g -fPIC -save-temps -fvisibility=hidden -std=gnu++17 -MD -MT src/CMakeFiles/yomm2.dir/yomm2.cpp.o -MF CMakeFiles/yomm2.dir/yomm2.cpp.o.d -o CMakeFiles/yomm2.dir/yomm2.cpp.o -c /home/jll/dev/yomm2/src/yomm2.cpp

This is official cmake behavior.

I have already made most of these changes (I have not obfuscated YOMM2_API yet) in a commit, that I will push to your PR. Also I added GitHub jobs for shared builds. Have you enabled the option that allows reviewers to add commits?

iboB commented 1 year ago

I think that you did not build the test suite ;-)

Check. I didn't build the test suite. The automatically downloaded distro of boost doesn't have Boost.Test and I thought I can do without it. :)

I wonder, however, if YOMM2_API should be obfuscated, like the helper macros here.

The obfuscation sounds good. I usually use an I_ prefix for "obfuscated" macros, but I like the lowercase style.

I don't think that BUILDING_YOMM2 is needed

That's true. I didn't even know of DEFINE_SYMBOL. In this case yomm2_EXPORTS can be used instead.

Note that BUILDING_YOMM2 has certain benefits (which are somewhat obscure and may well never come into play):

Here we're talking about a single .cpp file, though. If that's unlikely to change, I'd suggest ditching the definition from CMake entirely and just adding #define BUILDING_YOMM2 1 as the first line in yomm2.cpp

Have you enabled the option that allows reviewers to add commits?

Edits by maintainers are allowed.

jll63 commented 1 year ago

Here we're talking about a single .cpp file, though. If that's unlikely to change, I'd suggest ditching the definition from CMake entirely and just adding #define BUILDING_YOMM2 1 as the first line in yomm2.cpp

Good idea! In fact we can also dispense with BUILDING_YOMM2: have core.hpp define yOMM2_API to import, if it is not already defined; and have yomm2.cpp define it for export, before including core.hpp.

I pushed a commit to your PR. Please review it, and, if it looks good, squash it into your commit, and rebase.

iboB commented 1 year ago

Besides that small tidbit above, it looks good to me 👍