Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In an internal function called from a virtual function, do not call virtual functions. #16741

Merged
merged 1 commit into from Mar 9, 2024

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Mar 9, 2024

Finding this bug took probably on the order of half a week of actual work, for the addition of 34 characters. My many failed attempts at figuring out what is going on are chronicled in geodynamics/aspect#5421 and geodynamics/aspect#5577 .

The root cause here is that in ASPECT, we derive a class from SphericalManifold in which we create a sphere with topography (the Earth) by stacking operations in the same way as we often do with manifolds: You just concatenate operations to form the pull back and push forward operations. As a consequence, the overriding functions then call back into the functions in the base class. Then, our implementation of functions in SphericalManifold calls internal functions that do certain things and, in at least one case, further delegate to yet another function -- which is virtual! So we end back up in user space, apply our own secondary transformation one more time, and then call back into the SphericalManifold class. This is not desirable. The internal function only calls the other function for convenience, and so it needs to call the one in SphericalManifold, not the one in user space.

This patch fixes this. I cannot express how relieved I am to have fixed this.

(For the purposes of cross linking: We have had trouble with this class before. See #16242 and what is linked from there.)

@bangerth
Copy link
Member Author

bangerth commented Mar 9, 2024

@gassmoeller FYI

@kronbichler
Copy link
Member

The failing tests are addressed by #16740, so let us merge this.

@kronbichler kronbichler merged commit f973d2f into dealii:master Mar 9, 2024
15 of 16 checks passed
@bangerth bangerth deleted the virtual branch March 9, 2024 14:49
@gassmoeller
Copy link
Member

Wow, thanks for figuring this out! The wonders of polymorphism I guess :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants