feat: precompiled all #762

dherrera-fb · 2022-08-15T09:35:58Z

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:

All implementations are tagged as inline
One can still choose multi-header or single-header.
Else
Implementations are not tagges as inline
There is a single Precomplie.cpp file that compiles all implementations
The cmake target becomes a static library
The inl files are not included in the headers

This considerably improves compile-time but may hurt binary size.

Closes #338

- 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 · 2022-08-15T10:01:20Z

Codecov Report

Merging #762 (28d55aa) into main (c57000e) will decrease coverage by 0.40%.
The diff coverage is 98.63%.

@@            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

Impacted Files	Coverage Δ
include/CLI/App.hpp	`100.00% <ø> (+0.64%)`	⬆️
include/CLI/Option.hpp	`99.52% <ø> (-0.15%)`	⬇️
include/CLI/StringTools.hpp	`100.00% <ø> (ø)`
include/CLI/Validators.hpp	`100.00% <ø> (+0.60%)`	⬆️
include/CLI/impl/StringTools_inl.hpp	`95.51% <95.51%> (ø)`
include/CLI/impl/Validators_inl.hpp	`98.55% <98.55%> (ø)`
include/CLI/impl/Split_inl.hpp	`98.70% <98.70%> (ø)`
include/CLI/impl/Option_inl.hpp	`98.74% <98.74%> (ø)`
include/CLI/impl/App_inl.hpp	`98.80% <98.80%> (ø)`
include/CLI/impl/Config_inl.hpp	`99.14% <99.14%> (ø)`
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

henryiii

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.

henryiii · 2022-08-15T13:59:04Z

CMakeLists.txt

@@ -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")


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.

Agree. Pushed a commit to make it off by default

henryiii · 2022-08-15T14:15:05Z

Ahh, readability-avoid-const-params-in-decls is something I hadn't thought of. const T x and T x are the same thing if T is a pass-by-copy type, but adding the const helps the author of the function not change x. So those can be removed from the declarations but not the definitions. (Well, technically, the definitions too since they only help authors working on the definition).

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!

dherrera-fb · 2022-08-15T14:16:56Z

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.

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.

dherrera-fb · 2022-08-15T14:24:24Z

CI doesn't yet have checks for building precompiled mode. Maybe we should add that before merging?

henryiii · 2022-08-23T14:25:47Z

@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.

phlptp · 2022-08-23T15:18:11Z

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.

dherrera-meta and others added 9 commits August 9, 2022 14:46

[precompile] Split Option.hpp

dd628bf

[precompile] Split Config.hpp

37669a6

[precompile] Split Formatter.hpp

541fc2e

[precompile] Split Split.hpp

5248573

[precompile] Split StringTools.hpp

bd62587

[precompile] Split Validators.hpp

3d163c2

[precompile] Use a single Precompile.cpp

cc32085

style: pre-commit.ci fixes

768097c

dherrera-fb mentioned this pull request Aug 15, 2022

Plan for precompiled mode #338

Closed

henryiii reviewed Aug 15, 2022

View reviewed changes

Make CLI11_PRECOMPLED off by default

143ea42

henryiii and others added 4 commits August 15, 2022 10:35

ci: drop failing style check

893a1c8

style: pre-commit.ci fixes

d8b23f3

ci: add testing for pre-compiled mode

3b3d41b

ci: fix GCC 4.8 missing dir issue

28d55aa

phlptp approved these changes Aug 23, 2022

View reviewed changes

henryiii merged commit c7aff0e into CLIUtils:main Aug 23, 2022

github-actions bot added needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Aug 23, 2022

henryiii removed needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Sep 14, 2022

henryiii changed the title ~~All contributors/precompiled all~~ feat: precompiled all Sep 14, 2022

henryiii added the needs changelog Hasn't been added to the changelog yet label Sep 14, 2022

henryiii added the needs README Needs to be mentioned in the README label Sep 14, 2022

henryiii removed needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: precompiled all #762

feat: precompiled all #762

dherrera-fb commented Aug 15, 2022

codecov bot commented Aug 15, 2022 •

edited

Loading

henryiii left a comment •

edited

Loading

henryiii Aug 15, 2022

dherrera-fb Aug 15, 2022

henryiii commented Aug 15, 2022

dherrera-fb commented Aug 15, 2022

dherrera-fb commented Aug 15, 2022

henryiii commented Aug 23, 2022

phlptp commented Aug 23, 2022

feat: precompiled all #762

feat: precompiled all #762

Conversation

dherrera-fb commented Aug 15, 2022

codecov bot commented Aug 15, 2022 • edited Loading

Codecov Report

henryiii left a comment • edited Loading

Choose a reason for hiding this comment

henryiii Aug 15, 2022

Choose a reason for hiding this comment

dherrera-fb Aug 15, 2022

Choose a reason for hiding this comment

henryiii commented Aug 15, 2022

dherrera-fb commented Aug 15, 2022

dherrera-fb commented Aug 15, 2022

henryiii commented Aug 23, 2022

phlptp commented Aug 23, 2022

codecov bot commented Aug 15, 2022 •

edited

Loading

henryiii left a comment •

edited

Loading