Conversation
WeiqunZhang
requested review from
asalmgren,
atmyers,
drummerdoc,
jmsexton03 and
maximumcats
|
The issue has been addressed by #1836 for Mac. |
|
@WeiqunZhang since this PR gets rid of weak-symbols, is it ok for everyone to continue to pursue it? #1836 is a pretty invasive work-around/hack for macOS and does not solve the general unavailability of weak symbols on Windows. I would like to remove all linker-magic on Linux and macOS, too since this simplifies downstream dependency management, and the PR here would make that possible. Totally orthogonal to the weak symbols, we need to handle globals manually on Windows, which is not related to this PR: #1847 (comment) |
|
I was OK with this from the Castro side. It would have required a slight bit of reshuffling since our LevelBld* object is not currently visible from main() and thus could not be directly handed to the Amr constructor, but it should be straightforward to adjust that if we want to do this. |
|
We just chatted on Slack and continue with this PR 🎉 |
|
You can add this line to the main.cpp, I considered adding this line a Header in amrex, but decided not to do it. |
In Amr class, we need to get LevelBld* and use it to build AmrLevels. Previously, Amr called a function `LevelBld* getLevelBld()` to get that. The problem was that `getLevelBld` was not defined by amrex but it was being called by amrex. This caused issues in building amrex as a shared library on Mac and Windows, because they do not support weak symbols. In this PR, the constructors of Amr now take `LevelBld*` as an argument. This is unfortunately a breaking change. However, it's a compile time failure and it's easy to fix.
cgilet
left a comment
cgilet
left a comment
There was a problem hiding this comment.
Works for IAMR with the one line addition to main.cpp
|
@jrood-nrel Is PeleC ready for the change? |
|
Yes, we will update PeleC when you merge this. |
5 participants
In Amr class, we need to get LevelBld* and use it to build AmrLevels.
Previously, Amr called a function
LevelBld* getLevelBld()to get that.The problem was that
getLevelBldwas not defined by amrex but it was beingcalled by amrex. This caused issues in building amrex as a shared library
on Mac and Windows, because they do not support weak symbols.
In this PR, the constructors of Amr now take
LevelBld*as an argument.This is unfortunately a breaking change. However, it's a compile time
failure and it's easy to fix.
The following codes need updates: