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
<!--e57a25ab6845829454e8d69fc972939a-->No more conflicts as of last run.
Concept ACK I didn't do this initially because I didn't expect tinyformat to be an actually maintained library (as well as not really understanding submodules very well at the time), but in retrospect this is better.
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
Repeating @MarcoFalke's concern here for visibility:
Seems like a lot of files to pull in just to make it a subtree, no? Imo tinyformat is really meant to be a single file just to be copied into the project." #13842 (comment)
I have a mild preference for automatic checks of consistency with upstream, but #13846 exists as an alternative.
20 | @@ -21,7 +21,7 @@ endif 21 | 22 | BITCOIN_INCLUDES=-I$(builddir) $(BDB_CPPFLAGS) $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS) $(CRYPTO_CFLAGS) $(SSL_CFLAGS) 23 | 24 | -BITCOIN_INCLUDES += -I$(srcdir)/secp256k1/include 25 | +BITCOIN_INCLUDES += -I$(srcdir)/secp256k1/include -I$(srcdir)/tinyformat
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?
Concept ACK
Seems like a lot of files to pull in just to make it a subtree, no? Imo tinyformat is really meant to be a single file just to be copied into the project."
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 ~2~ 1: https://github.com/Empact/bitcoin/tree/tinyformat-subdir/src/tinyformat
The other changed files relate mostly to redirecting tinyformat.h includes to utilstrprintf.h.
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.
181 | @@ -182,6 +182,7 @@ BITCOIN_CORE_H = \ 182 | util.h \ 183 | utilmemory.h \ 184 | utilmoneystr.h \ 185 | + utilstrprintf.h \
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 utilstrprintf.h to tinyformat.h
git-subtree-dir: src/tinyformat
git-subtree-split: 689695cf58700e6defe3741829564cd682d5ae57
Introduce modifications to tinyformat via src/tinyformat.h
This includes changes in:
695041e4952ea40e0 - util: Update tinyformat
1b8fd35aadfad6a1e - Make tinyformat errors raise an exception instead of assert()ing
9b6d4c5cdc1ad7b12 - Move strprintf define to tinyformat.h
6e5fd003e04b81115 - Move `*Version()` functions to version.h/cpp
9eaa0afa6ec5d3dd0 - tinyformat: force USE_VARIADIC_TEMPLATES
b651270cd6bfdd6d7 - 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:
64fb0ac016c7fd01c - Declare single-argument (non-converting) constructors "explicit"
4d9b4256d89d1f7c6 - 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.
Closing (see #13846 (comment))