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

Avoid boost::optional/variant in favor of std_cxx17::variant/optional. #14392

Merged
merged 1 commit into from Nov 22, 2022

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Nov 4, 2022

I don't have CGAL installed, so can't test whether this actually works. But the uses of the old boost classes appear all internal to me, not in the interfaces to CGAL, and so I think this should work.

/rebuild

@fdrmrc
Copy link
Contributor

fdrmrc commented Nov 4, 2022

This doesn't compile since the return type of CGAL::intersection is really a boost::optional<boost::variant<>>.
When I added these utilities, I used decltype(auto) (as per the CGAL doc), but in the review process we decided to be explicit about the return type.

@bangerth
Copy link
Member Author

bangerth commented Nov 9, 2022

Ah, that's a bummer. How do people feel -- is it worth converting the CGAL types to std:: types for consistency throughout our code base, or should we just leave things as is?

@fdrmrc
Copy link
Contributor

fdrmrc commented Nov 22, 2022

Thanks for cleaning this. Since CGALWrappers require C++17, you could also replace std_cxx17 with just std.

@bangerth
Copy link
Member Author

Yes, it took me a number of tries to get this right, but it compiles now. It's ready from my side to be merged.

If you all would allow me to, let's leave it with std_cxx17 for the moment. We'll do a bulk replace when we will require C++17 everywhere, but for the moment it might create more confusion than help to use different conventions in different places.

@drwells drwells merged commit 378d42d into dealii:master Nov 22, 2022
@bangerth bangerth deleted the cxx17 branch November 22, 2022 18:02
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

4 participants