-
Notifications
You must be signed in to change notification settings - Fork 34
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
Import std module #46
Conversation
Macro guard out all #includes of std headers Reorder catch2.hpp to be included first as it includes std headers
Note the option command had the off in the wrong place, it should be at the end so I fixed that
Oh i did change on other thing. You had
I believe that the ordering is wrong here, see https://cmake.org/cmake/help/latest/command/option.html#option so I changed it to
Note the default is OFF so you could omit that but I like it being explicit |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 338 356 +18
=========================================
+ Hits 338 356 +18 ☔ View full report in Codecov by Sentry. |
Wow. Thanks. I've been wanting to do this for some time. It's a large change, so it'll take a bit of time for me to go through all the details. However, there are two issues that I see at a short glance: No change to the CI build scripts, so none of your changes are tested. Please fix that. You changed the include order in the tests so that Catch2 is included before the feature being tested. That is, IMO, totally wrong. Don't do that. You always want to include the functionality being tested first, to ensure you don't rely on any accidentally included dependency. A related, but different thing. I want to make this library itself a module, but I lack the knowledge and time to do so. Do you have the time (you appear to have the knowledge)? It'd be awesome if you could assist with that. (as a separate PR). |
1) Change to CI build scriptsThis was mainly because I have zero experience in these sort of scripts. Is this a separate appveyor.yml file or should it be added to the current one? I'm also not sure how to test them, can I run them on my branch? 2) Reorder of <catch2.h>I completely agree with you that this is the wrong thing to do. However it was necessary to get the tests to build. Catch2 does not have a similar mechanism to build optionally with Therefore is you include it second you get the sequence
This fails on both MSVC and clang. Note that the reverse works on both. According to the standard this should work.
The machinery that does the import can check to see if there is a definition already present and ignore it if there is one. Anyway the upshot is that I could do some macro thing like
Although I think this is ugly Another possibility is to have file equivalent to
So there would be one of these cpp files per header. 3) Making a module of strong_typeI have done much less of this. In fact a year or so ago I immediately ran into the issue of including other libraries hence my quest to "fix" them. However I'm willing to have a go. I had a couple of comments/questions a) There is no such thing as a "interface" or header only module I'm assuming you are OK with this One sad thing is that I believe MSVC, Clang and GCC all use different file extensions :-( BTW are you going to CPPCon this year, in a week in fact, if so hope to see you there. |
1 & 2. Hmm. I see. I'll try to do something. A separate .cpp file for each header is a bit of an abomination, but maybe it's OK to have as something that is only used by CI.
|
Note there is an ugly way to work around the ordering issue of the catch header. The problem is that no compiler currently supports
and they probably won't for a while. The work arounds I know of are
Both of these mechanisms were discussed here microsoft/STL#3195 Note MS are still doing a lot of work on modules in fact microsoft/STL#4375 was got me looking at strong_type again. It removed another bunch of ugliness |
Hmm, odd. I handled this manually because I wanted to ensure the CI was running with support for "import std;" before merging, and apparently github lost track of where the change came from. Anyway, the support just landed in main. Huge thank you for your contribution, and apologies for taking so long. |
These changes optionally all the library to be built and consumed using
import std;
rather than classic old style#include <meow>
. This currently works for Visual Studio and Clang using libc++. It required the latest CMake 3.30 and Clang versions.This is not an attempt to create a module from the library, which would be nice to have, only to use the std module.
In CMake everything is guarded by a
STRONG_TYPE_IMPORT_STD_LIBRARY
variable. In code stuff is guarded by a macro of the same name. By default STRONG_TYPE_IMPORT_STD_LIBRARY is OFF is CMake so by default everything works exactly as now.Many of the code changes are of the form
Note both MSVC and clang can do an old style
#include
before animport std;
but both give error when you do the reverse. For this reason in the tests the#include <catch2.hpp>
had to be moved to be the first include as it does unguarded#include <meow>
.So as as user I can now
#include strong_type.hpp
with STRONG_TYPE_IMPORT_STD_LIBRARY defined and useimport std;
One advantage of doing this is that the std module doesn't leak macros and global namespace things like
uint32_t
so you now know that you don't rely on things like that. I did fix a couple of cases where the library usedptrdiff_t
from the global namespace.Note MSVC does have a bug where it does leak stuff into the GMF, see microsoft/STL#1694 for details.
Anyway hope this is of interest, let me know if you want changes or documentation.