Include tinyformat as a subtree #13845
Conversation
| No more conflicts as of last run. |
|
Concept ACK |
|
utACK – LGTM |
|
labeling this for after the 0.18 branch-off, I don't think this makes sense to do last-minute for 0.17 |
|
@laanwj Agreed |
|
Repeating @MarcoFalke's concern here for visibility:
I have a mild preference for automatic checks of consistency with upstream, but #13846 exists as an alternative. |
ffc28d8 to
f5ee845
Compare
src/Makefile.am
Outdated
| @@ -21,7 +21,7 @@ endif | |||
|
|
|||
| BITCOIN_INCLUDES=-I$(builddir) $(BDB_CPPFLAGS) $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS) $(CRYPTO_CFLAGS) $(SSL_CFLAGS) | |||
|
|
|||
| BITCOIN_INCLUDES += -I$(srcdir)/secp256k1/include | |||
| BITCOIN_INCLUDES += -I$(srcdir)/secp256k1/include -I$(srcdir)/tinyformat | |||
There was a problem hiding this comment.
tinyformat.h is only included directly from utilstrprintf.h; seems more sensible to just have utilstrprint.h #include <tinyformat/tinyformat.h> and not extend the include path?
After seeing the number of files added here I think I'm starting to agree with @MarcoFalke. Meta-nit: In general it's a bad idea to open multiple competing PRs, because it distributes the discussion over multiple places, which leads to confusion. |
|
Here are the files included in the subtree - 10 in total: https://github.com/Empact/bitcoin/tree/tinyformat-subtree/src/tinyformat #13846 cuts this to The other changed files relate mostly to redirecting tinyformat.h includes to utilstrprintf.h. |
f5ee845 to
7823f5c
Compare
|
The two PRs is a bit of a pain. I think I have a mild preference for the 10-files-but-automatic-checks over 2-files-but-manual-checks, but am not really bothered either way. The 46-files-changed is mostly just churn to pull out the customisations into utilstrprintf.h. You could avoid that by renaming utilstrprintf.h back to tinyformat.h (which then includes tinyformat/tinyformat.h). If I'm counting right, doing that gets it down to 24 changes: 10 src/tinyformat/, src/tinyformat.h, dev-notes.md, copyright_header.py, Makefile.am, src/Makefile.am, 6 lint files, travis.yml, and src/primitives/block.{h,cpp}. The block.{h,cpp} changes are just to directly include some standard headers so could also be skipped. |
src/Makefile.am
Outdated
| @@ -182,6 +182,7 @@ BITCOIN_CORE_H = \ | |||
| util.h \ | |||
| utilmemory.h \ | |||
| utilmoneystr.h \ | |||
| utilstrprintf.h \ | |||
There was a problem hiding this comment.
Is this change needed/useful?
|
One option is to split this into 2 prs: the extraction of utilstrprintf.h and the move to a subdir (which would be pretty trivial at that point). |
|
Agree with @ajtowns, rename |
3ae0361 to
5e84670
Compare
git-subtree-dir: src/tinyformat git-subtree-split: 689695cf58700e6defe3741829564cd682d5ae57
Introduce modifications to tinyformat via src/tinyformat.h This includes changes in: 695041e - util: Update tinyformat 1b8fd35 - Make tinyformat errors raise an exception instead of assert()ing 9b6d4c5 - Move strprintf define to tinyformat.h 6e5fd00 - Move `*Version()` functions to version.h/cpp 9eaa0af - tinyformat: force USE_VARIADIC_TEMPLATES b651270 - util: Throw tinyformat::format_error on formatting error This does not include changes below, and it protects against future unintended modification of upstream code via git-subtree-check: 64fb0ac - Declare single-argument (non-converting) constructors "explicit" 4d9b425 - Fix typos Note I excluded only the tinyformat cpp from the boost lint checks out of an abundance of caution - these files are not actually built into bitcoin so are not at risk of pulling in unwanted dependencies, whereas the header, and other dependencies might be.
5e84670 to
8bb7c25
Compare
|
Rebased for #13863 (actually, regenerated subtree and cherry-picked changes) |
|
Closing (see #13846 (comment)) |
This adds the existing c42f/tinyformat@689695c of tinyformat as a subtree, and extracts the modifications that had been made to a utilstrprintf.h wrapper file which is included instead of tinyformat.h directly.
The benefits being that this should be easier to verifiably update and clearer what changes are being introduced, while making it easier to identify tinyformat.h as a dependency rather than a project file, and apply special treatment relative to that fact.
This re-opens #13842