-
Notifications
You must be signed in to change notification settings - Fork 56
Implement initial version of C++20 module boost.type_index
#15
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
Conversation
@anarthal I'd appreciate comments on this PR on modules (as you proposed at boostorg/core#191 (comment)) BTW, does the MSVC with |
AFAIR I made it work at some point. Subsequent changes may have broken it again though. |
CMakeLists.txt
Outdated
add_library(boost_type_index) | ||
target_sources(boost_type_index PUBLIC | ||
FILE_SET modules_public TYPE CXX_MODULES FILES | ||
${CMAKE_CURRENT_LIST_DIR}/modules/type_index.cppm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convention is boost_type_index.cppm
modules/type_index.cppm
Outdated
#include <boost/throw_exception.hpp> | ||
|
||
#ifdef BOOST_TYPE_INDEX_USE_STD_MODULE | ||
import std; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically not allowed. A global module fragment can only contain preprocessor directives, so you should place this in an include. AFAIR clang doesn't complain and MSVC issues a warning. The change I'm suggesting doesn't imply any change in behavior.
# pragma clang diagnostic ignored "-Winclude-angled-in-module-purview" | ||
#endif | ||
|
||
#include <boost/type_index.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you include your library files in the purview, you need to be extra careful with if-defing out all third party includes. I've seen a <boost/config.hpp>
without an ifdef, which would cause any names defined by it (including the ones poured by including STL headers) into the named module.
|
||
export module boost.type_index; | ||
|
||
#ifdef __clang__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSVC has a similar warning
include/boost/type_index.hpp
Outdated
|
||
#include <boost/type_index/detail/config.hpp> | ||
|
||
#include <boost/config.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're including this in the purview, you need to ifdef this include out.
#include <type_traits> | ||
#endif | ||
|
||
#include <boost/throw_exception.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to ifdef-out other Boost headers, too.
|
||
#if !defined(BOOST_USE_MODULES) || defined(BOOST_TYPE_INDEX_INTERFACE_UNIT) | ||
|
||
#include <boost/config.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to ifdef-out other Boost headers, too.
#if !defined(BOOST_USE_MODULES) || defined(BOOST_TYPE_INDEX_INTERFACE_UNIT) | ||
|
||
#include <boost/type_index/runtime_cast/detail/runtime_cast_impl.hpp> | ||
#include <boost/throw_exception.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boost header
module; | ||
|
||
#include <version> | ||
#include <cstddef> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work under MSVC? Last time I checked, mixing #include <cstddef>
and import std
failed. If you encounter this problem, #include <stddef.h>
doesn't cause this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to workaround too many issues on MSVC, so I'd say that "boost.any module does not support MSVC for now".
add_library(boost_type_index INTERFACE) | ||
set(__scope INTERFACE) | ||
endif() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something or is there no ${CMAKE_CURRENT_SOURCE_DIR}/test/CMakeLists.txt
? I'd advise to remove this check if you know for sure that the file should be present. AFAIK this is generated by Peter's CMake generator so it's compatible with libraries that don't run their tests via CMake.
#include <boost/type_index...
is now implicitly doesimport boost.type_index
if the modules are supportedAll the library internals now have unconditional module level linkage.
Significant differences from https://anarthal.github.io/cppblog/modules3:
BOOST_TYPE_INDEX_USE_STD_MODULE
macro switch forimport std;
/includes
while building module. This allows to use module in C++20 and even without usablestd
module.