Skip to content

Replace boost type_traits with stl type_traits. #1373

New issue

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

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

Already on GitHub? # to your account

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gogagum
Copy link

@gogagum gogagum commented Feb 7, 2025

With C++14, is possible to replace Boost.TypeTraits library usages with standard type_traits. This PR removes direct Boost.TypeTraits dependency in Geometry module.

@@ -65,10 +64,10 @@ class serialization_storage
}
T * address()
{
return static_cast<T*>(m_storage.address());
return static_cast<void*>(&m_storage);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awulkiew is this fine?

Copy link
Member

@awulkiew awulkiew Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::aligned_storage is deprecated in C++23. AFAIR boost version is superior to the std one but I can't remember why besides the fact that it doesn't force us to cast address of the storage. A proposed replacement is something like this:

alignas(T) std::byte storage[sizeof(T)];

but it requires C++17 and would require careful testing. So I'd prefer to keep boost::aligned_storage for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, here https://en.cppreference.com/w/cpp/types/aligned_storage the required use of std::launder is mentioned. This change can introduce some unwanted effects. I guess I strongly suggest to keep the old version. :)

Copy link
Collaborator

@barendgehrels barendgehrels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me, one question to Adam

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this look OK to me.

@@ -65,10 +64,10 @@ class serialization_storage
}
T * address()
{
return static_cast<T*>(m_storage.address());
return static_cast<void*>(&m_storage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, here https://en.cppreference.com/w/cpp/types/aligned_storage the required use of std::launder is mentioned. This change can introduce some unwanted effects. I guess I strongly suggest to keep the old version. :)

@barendgehrels
Copy link
Collaborator

@gogagum , will you address the one review action item indicated by @awulkiew ?

@barendgehrels
Copy link
Collaborator

@gogagum will you update it or can we close it? (we still can remove the dependency later)

@gogagum
Copy link
Author

gogagum commented May 9, 2025

@barendgehrels I am going to finish this work.

@gogagum gogagum marked this pull request as draft May 9, 2025 19:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants