feat: precompiled all #762
Conversation
- Add C11_COMPILE cmake option that creates a static lib instead of header-only - Add C11_INLINE macro that depends on C11_COMPILE - Split App.hpp into App.hpp and impl/App_inl.hpp - Add App.cpp that compiles App_inl.hpp into an object file - CMake modifications to handle impl headers differently for sinlge-header, headers-only, and compiled versions
Codecov Report
@@ Coverage Diff @@
## main #762 +/- ##
==========================================
- Coverage 99.46% 99.06% -0.41%
==========================================
Files 12 16 +4
Lines 3958 3964 +6
==========================================
- Hits 3937 3927 -10
- Misses 21 37 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
It's hard to review the diff, since everything moved. The procedure outlined sounds correct, though. I think we should block out merging any other PRs until this goes in. Does this include any/all recent changes?
CC @phlptp
PS: Don't worry about coverage. We lost 100% at some point when we lost coverage reporting for a while. We'll bring it back up eventually.
CMakeLists.txt
Outdated
| @@ -77,6 +77,7 @@ endif() | |||
|
|
|||
| option(CLI11_WARNINGS_AS_ERRORS "Turn all warnings into errors (for CI)") | |||
| option(CLI11_SINGLE_FILE "Generate a single header file") | |||
| option(CLI11_PRECOMPILED "Generate a precompiled static library instead of a header-only") | |||
There was a problem hiding this comment.
I think I'd recommend setting the default to OFF at first. That way it could be opt-in for a while, and then maybe the default could swap in 3.0.
There was a problem hiding this comment.
Agree. Pushed a commit to make it off by default
|
Ahh, Perfectly fine to just disable the check for now, and re-add it in a separate PR. Diff should stay small for this sort of moving code! |
I think this contains all recent changes. It was based off main. Last commit is c57000e from Jul 5th. I think that is still main as of now. Review is indeed hard for such massive changes. I tried to split it into commits to tackle each file separately. But I don't think there is much more I can do. |
|
CI doesn't yet have checks for building precompiled mode. Maybe we should add that before merging? |
|
@phlptp Okay with this? I'm okay to merge then see if anything needs polishing. If we are lucky, maybe we'll have ppl on main that will help catch anything our tests do not. |
|
It looks ok to me, It is a big change and I suspect will need a bit of polish but that is going to be difficult to check at present, so I think I am good with a merge then we can figure out what cleanup is necessary after a bit more testing. |
This is the full implementation precompiled option. It splits all header files into x.hpp and x_inl.hpp, where the first has only declarations and trivial inlines and the latter has the implementations.
If the cmake variable CLI11_PRECOMPILED is false:
Else
This considerably improves compile-time but may hurt binary size.
Closes #338