Use PIMPL in our muParser implementation. #13577
Conversation
|
This is a very interesting clang-tidy complaint:
but: we do this all the time, see Lines 954 to 956 in 556c6c6 Perhaps the extra implicit conversion is getting in the way. |
drwells
force-pushed
the
muparser-pimpl
branch
2 times, most recently
from
cbafe97
to
d031294
Compare
|
adding |
drwells
force-pushed
the
muparser-pimpl
branch
2 times, most recently
from
eb46097
to
7af6209
Compare
|
I still think #11609 would have been great. You can also tell deal.II to use the external muparser and your case will work (but it is annoying, I know). I am not sure about this workaround. Carrying patches on top of it makes it harder to update. |
|
I don't like that in the future we will have to remember to do this but I think its still the right choice. We haven't updated muParser since 2015 - how about we update the bundled copy now and then apply this patch? That way we won't have to worry about this until 2029 or so. Another option would be to define a preprocessor symbol indicating that we are using the bundled version and then check, with a test, that the correct symbols exist (e.g., |
I think that makes sense. Does your current PR work with an external muparser as well? |
|
Can do! Yes, this works with an external muParser. |
|
What do others think about this PR, @dealii/dealii ? |
| /** | ||
| * Destructor. | ||
| */ | ||
| virtual ~FunctionParser() override; |
There was a problem hiding this comment.
Why do you no longer need this?
There was a problem hiding this comment.
We can get rid of this since we are now using PIMPL: the default destructor, generated in the header, doesn't need to call ~mu::Parser() but instead calls ~internal::muParserBase() which is virtual (and overridden in the source file).
There was a problem hiding this comment.
Ah, never mind. It's because you now store a pointer to the base class which is visible in the header file.
|
I'm ok with wrapping header files. |
|
This now relies on #13581 - lets not merge until that is in. |
|
This can now be reviewed again since the other PR is in. |
|
The windows link problem is really baffling. I'll take a closer look at this this week. |
This should not have been ported to the new directory.
We should always set up the linkage options ourselves (i.e., always use static linkage) and not let muParser try to set things up for dynamic linkage in case we forget. This only applies to MSVC.
| // Since this header is not installed this behavior change is purely internal. | ||
| #define API_EXPORT_CXX | ||
| #define MUPARSER_LOCAL | ||
| #define API_EXPORT(TYPE) TYPE |
There was a problem hiding this comment.
I'm not sure how this worked on master, but we need to completely disable muParser's own MSVC DLL linkage fixes to get things to work consistently.
|
Would someone take another look? |
|
I'm ok with this. If no one objects, I'd go ahead and merge. |
In #11609 we decided that we would not install the muParser headers. Our bundled version of muParser is a bit odd because its compiled directly into deal.II and the headers are not publicly available, but
deal_II.soet al define all the standard muParser symbols: i.e., its a copy of muParser that conflicts with 'real' muParser installations but cannot be used as one. Hence, people unaware of how this works get weird crashes or link errors (like my student today).This PR resolves that inconsistency by properly namespacing our own version: all the symbols now go in
dealii::muwhich will permit us to safely link against another copy of muParser. I also removed the need to have any muParser stuff in the headers at all by using the PIMPL tactic.