Move src/tinyformat.h to src/tinyformat/tinyformat.h #13846

pull Empact wants to merge 1 commits into bitcoin:master from Empact:tinyformat-subdir changing 13 files +1087 −1057
  1. Empact commented at 5:26 am on August 2, 2018: member

    For consistency with other bundled dependencies. Having it in the top-level directory makes it clear that it is a library that we’re including rather than a file that is a part of the project.

    tinyformat/tinyformat.h and tinyformat/README.md are drawn from the existing sha reference: https://github.com/c42f/tinyformat/tree/689695cf58700e6defe3741829564cd682d5ae57

    Existing modifications to tinyformat are moved to utilstrprintf.h, clearly separating upstream and project code.

    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, which are cosmetic or apparently unintentional: 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.

  2. Empact commented at 5:28 am on August 2, 2018: member
    This is an alternative to #13845. This includes just the header and readme, while #13845 includes the full subtree - more automatic checking is possible with that, but at the cost of including support files.
  3. laanwj commented at 8:10 am on August 2, 2018: member
    Concept ACK
  4. DrahtBot commented at 8:19 am on August 2, 2018: member
  5. laanwj added the label Upstream on Aug 2, 2018
  6. Empact force-pushed on Aug 2, 2018
  7. laanwj commented at 12:46 pm on August 7, 2018: member
    Closing this in favor of #13845, at least I’d say that if we’re going to move it anyhow go the full yard and make it a subtree.
  8. laanwj closed this on Aug 7, 2018

  9. laanwj reopened this on Aug 7, 2018

  10. laanwj commented at 12:48 pm on August 7, 2018: member
    Reopening because of @MarcoFalke’s concern, agree that #13845 pulls in a bizarrely high number (46!) of files. And the whole intent of tinyformat.h is to have a file that can be easily copied into a project. It’s not clear it is worth it.
  11. laanwj added this to the milestone 0.18.0 on Aug 7, 2018
  12. Empact force-pushed on Aug 12, 2018
  13. in src/tinyformat/README.md:3 in 8e603876f6 outdated
    0@@ -0,0 +1,452 @@
    1+# tinyformat.h
    2+
    3+## A minimal type safe printf() replacement
    


    MarcoFalke commented at 11:58 pm on August 12, 2018:
    No need to add a third party readme. This will show up in all history/logs and greps.
  14. MarcoFalke commented at 11:59 pm on August 12, 2018: member
    Concept ACK
  15. Empact force-pushed on Aug 13, 2018
  16. Empact commented at 2:07 am on August 13, 2018: member
    Dropped the readme. 👍
  17. in test/lint/README.md:20 in c5ee680fdf outdated
    16@@ -17,6 +17,7 @@ To use, make sure that you have fetched the upstream repository branch in which
    17 maintained:
    18 * for `src/secp256k1`: https://github.com/bitcoin-core/secp256k1.git (branch master)
    19 * for `src/leveldb`: https://github.com/bitcoin-core/leveldb.git (branch bitcoin-fork)
    20+* for `src/tinyformat`: https://github.com/c42f/tinyformat.git (branch master)
    


    MarcoFalke commented at 11:49 am on August 27, 2018:
    nit: Not a subtree
  18. MarcoFalke commented at 11:50 am on August 27, 2018: member
    utACK c5ee680fdfe806ab6ffbfca47f6cd28e4fb4e049
  19. Move src/tinyformat.h to src/tinyformat/tinyformat.h
    For consistency with other bundled dependencies. Having it in the top-level
    directory makes it clear that it is a library that we're including
    rather than a file that is a part of the project.
    
    tinyformat/tinyformat.h and tinyformat/README.md are drawn from the existing
    sha reference:
    https://github.com/c42f/tinyformat/tree/689695cf58700e6defe3741829564cd682d5ae57
    
    Existing modifications to tinyformat are moved to utilstrprintf.h, clearly
    separating upstream and project code.
    
    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, which are cosmetic or apparently unintentional:
    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.
    d48249e240
  20. Empact force-pushed on Aug 28, 2018
  21. Empact commented at 1:13 am on August 28, 2018: member
    Removed inaccurate test/lint/README mention and unnecessary test/lint/lint-includes.sh filter.
  22. MarcoFalke commented at 7:22 pm on August 29, 2018: member
    re-utACK d48249e2400487fb99a6bb15999eccd489a71318
  23. laanwj commented at 1:37 pm on August 31, 2018: member

    This was discussed at the 2018-08-30 IRC meeting and there was no agreement to do this at all.

     021:20 < wumpus> one topic I'd like to discuss is where to move tinyformat in the source tree, if we're going to do
     1                that at all, I hate it when there's two competing PRs open for something
     221:20  * jonasschnelli is lost in PRs
     321:20 < wumpus> #topic tinyformat move
     421:20 < wumpus> e.g.: [#13846](/bitcoin-bitcoin/13846/), [#13845](/bitcoin-bitcoin/13845/), or keep as is
     521:20 < gribble> [#13846](/bitcoin-bitcoin/13846/) | Move src/tinyformat.h to
     6                 src/tinyformat/tinyformat.h by Empact . Pull Request [#13846](/bitcoin-bitcoin/13846/) . bitcoin/bitcoin . GitHub
     721:20 < gribble> [#13845](/bitcoin-bitcoin/13845/) | Include tinyformat as a subtree by Empact . Pull
     8                 Request [#13845](/bitcoin-bitcoin/13845/) . bitcoin/bitcoin . GitHubAsset 1Asset 1
     921:21 < wumpus> I'm ok with all three options but not with leaving those PRs open forever
    1021:22 < jonasschnelli> The subtree looked to me after an overkill,... I would prefer [#13846](/bitcoin-bitcoin/13846/) (no strong opinion)
    1121:22 < gribble> [#13846](/bitcoin-bitcoin/13846/) | Move src/tinyformat.h to
    12                 src/tinyformat/tinyformat.h by Empact . Pull Request [#13846](/bitcoin-bitcoin/13846/) . bitcoin/bitcoin .
    1321:22 < wumpus> I guess MarcoFalke is not here?
    1421:23 < wumpus> I think he has the strongest opinion about it
    1521:23 < gmaxwell> would we really do a subtree for a single file?
    1621:23 < wumpus> no.
    1721:24 < wumpus> I think this is pretty much unnecessary, and certainly the subtree one contains lots of changes
    1821:24 < gmaxwell> seems like change for the sake of change to me.
    1921:24 < wumpus> too much of that
    2021:25 < achow101> I'm in favor of keeping it as is
    

    If there’s no agreement how to go forward here, I’d say it’s not extremely important to do, so I’m closing both PRs for now.

  24. laanwj closed this on Aug 31, 2018

  25. MarcoFalke commented at 2:48 pm on August 31, 2018: member
    Imo this could still be done the next time we touch tinyformat, but yeah no rush
  26. MarcoFalke locked this on Sep 8, 2021


Empact laanwj DrahtBot MarcoFalke

Labels
Upstream

Milestone
0.18.0


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me