Explicit symbol visibility - #2140
Conversation
ed7f7c7 to
3966938
Compare
1326432 to
34442de
Compare
|
@isuruf hello, I got to a more suitable solution where the same dllimport/dllexport mecanism is used for both msvc and mingw compilers, with a few changes about polys and GaloisField for which I'm not sure (due to subtle template export differences between the two compilers) |
|
@jschueller thanks for the PR - I pushed one small bugfix (4851205) and extended our tests to reproduce the bug (05ddd7d). |
|
Tick the box to add this pull request to the merge queue (same as
|
|
great, thanks! |
| return pow(base.get_basic(), exp.get_basic()); | ||
| } | ||
|
|
||
| inline void mp_pow_ui(Expression &res, const Expression &base, |
There was a problem hiding this comment.
mp_pow_ui seems like a weird name for Expression class. pow name seems enough.
There was a problem hiding this comment.
this is needed in usymenginepoly, else I get an error:
D:\a\symengine\symengine\symengine\polys\usymenginepoly.h(14,5): error C2665: 'SymEngine::mp_pow_ui': no overloaded function could convert all the argument types [D:\a\symengine\symengine\build\symengine\symengine.vcxproj]
| return true; | ||
| } | ||
|
|
||
| static bool _symengine_constants_initialized = _symengine_init_constants(); |
There was a problem hiding this comment.
Can you explain why this is needed?
There was a problem hiding this comment.
indeed, thats tricky, as we need constants to be explicitly exported now, this guarantees the initialization order of the constants storage: the _symengine_constants_initialized triggers the instanciation of the ConstantInitializer across all TUs which in turns allocates the constants (see also constants.cpp)
0abfcc8 to
cfa0a11
Compare
|
I think it would be great to get this in, you all have more experience dealing with symbol hiding on these platforms than me so I'll leave the technicalities to you. One question though: would it be feasible to add a (or replace an existent) CI job which exercises these changes? (and guards against regressions) |
|
ok, done, there was already a msvc job with shared libs, and I modified one mingw job to build shared libs too |
dce8c9c to
88b47c0
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
9704ab3 to
df43d40
Compare
This allows to export dll symbols on MinGW
…ck for GaloisField
|
Tick the box to add this pull request to the merge queue (same as
|
7c12367 to
94b2fce
Compare
|
@isuruf your changes to set default visibility to hidden exposed a few more places where SYMENGINE_EXPORT was missing which is fixed now, but it also exposed some issues on the ci with the new default BUILD_SHARED_LIBS=ON as this is an independent issue, lets discuss this in #2152 (you can inspect the ci failures there) |
|
@jschueller, let's add my changes back in with the default for |
|
Tick the box to add this pull request to the merge queue (same as
|
|
done, its enabled by default except when piranha is used because of its template instanciations (like hash_set iterators) creating ODR violations leading to crashing the tests |
This generalizes the use of SYMENGINE_EXPORT visibility macro to export all symbols for all windows compilers instead of using CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS in order to allow shared builds with mingw.