univalue: Avoid std::string copies #25714

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2207-univalue-types-🙉 changing 3 files +18 −12
  1. maflcko commented at 4:26 PM on July 26, 2022: member

    This shouldn't matter too much, unless a really large string is pushed into a json struct, but I think it also clarifies the code.

  2. maflcko added the label Refactoring on Jul 26, 2022
  3. fanquake commented at 1:11 PM on July 27, 2022: member
  4. DrahtBot cross-referenced this on Jul 29, 2022 from issue univalue: Remove unused and confusing set*() return value by maflcko
  5. DrahtBot commented at 10:30 PM on July 29, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  6. maflcko force-pushed on Aug 2, 2022
  7. maflcko force-pushed on Aug 2, 2022
  8. DrahtBot cross-referenced this on Aug 19, 2022 from issue Fix issues when calling std::move(const&) by maflcko
  9. DrahtBot added the label Needs rebase on Aug 31, 2022
  10. maflcko force-pushed on Sep 1, 2022
  11. fanquake requested review from ryanofsky on Sep 1, 2022
  12. in src/univalue/test/object.cpp:168 in fa67d7cafc outdated
     160 | @@ -160,6 +161,13 @@ void univalue_set()
     161 |      BOOST_CHECK(v.isStr());
     162 |      BOOST_CHECK_EQUAL(v.getValStr(), "zum");
     163 |  
     164 | +    {
     165 | +        std::string_view sv{"abc"};
     166 | +        UniValue j{sv};
     167 | +        BOOST_CHECK(j.isStr());
     168 | +        BOOST_CHECK_EQUAL(j.getValStr(), "abc");
    


    ryanofsky commented at 4:54 PM on September 1, 2022:

    In commit "univalue: string_view test" (fa67d7cafccb88b508e4eecdd9d66c2582647851)

    Would be exciting to check that it handles non-nul terminated strings, and embedded nuls correctly, too:

    diff --git a/src/univalue/test/object.cpp b/src/univalue/test/object.cpp
    index 02bcb5b7074..34b1c1cec48 100644
    --- a/src/univalue/test/object.cpp
    +++ b/src/univalue/test/object.cpp
    @@ -162,10 +162,11 @@ void univalue_set()
         BOOST_CHECK_EQUAL(v.getValStr(), "zum");
     
         {
    -        std::string_view sv{"abc"};
    +        std::string_view sv{"ab\0cd", 4};
             UniValue j{sv};
             BOOST_CHECK(j.isStr());
    -        BOOST_CHECK_EQUAL(j.getValStr(), "abc");
    +        BOOST_CHECK_EQUAL(j.getValStr(), sv);
    +        BOOST_CHECK_EQUAL(j.write(), "\"ab\\u0000c\"");
         }
     
         v.setFloat(-1.01);
    

    maflcko commented at 8:41 AM on November 7, 2022:

    Thanks, done

  13. in src/univalue/include/univalue.h:23 in fa1ce3ab42 outdated
      19 | @@ -20,10 +20,7 @@ class UniValue {
      20 |      enum VType { VNULL, VOBJ, VARR, VSTR, VNUM, VBOOL, };
      21 |  
      22 |      UniValue() { typ = VNULL; }
      23 | -    UniValue(UniValue::VType initialType, const std::string& initialStr = "") {
      24 | -        typ = initialType;
      25 | -        val = initialStr;
      26 | -    }
      27 | +    UniValue(UniValue::VType type, std::string str = "") : typ{type}, val{std::move(str)} {}
    


    ryanofsky commented at 5:02 PM on September 1, 2022:

    In commit "univalue: Avoid std::string copies" (fa1ce3ab42fd5fd761b934b8db9fbf9f78bde842)

    Probably doesn't matter in practice, but it seems like it could be less work for the compiler to call the default string constructor instead of the char* constructor

    diff --git a/src/univalue/include/univalue.h b/src/univalue/include/univalue.h
    index 54c60cada9a..6ef60b16e29 100644
    --- a/src/univalue/include/univalue.h
    +++ b/src/univalue/include/univalue.h
    @@ -20,7 +20,7 @@ public:
         enum VType { VNULL, VOBJ, VARR, VSTR, VNUM, VBOOL, };
     
         UniValue() { typ = VNULL; }
    -    UniValue(UniValue::VType type, std::string str = "") : typ{type}, val{std::move(str)} {}
    +    UniValue(UniValue::VType type, std::string str = {}) : typ{type}, val{std::move(str)} {}
         template <typename Ref, typename T = std::remove_cv_t<std::remove_reference_t<Ref>>,
                   std::enable_if_t<std::is_floating_point_v<T> ||                      // setFloat
                                        std::is_same_v<bool, T> ||                      // setBool
    

    maflcko commented at 12:37 PM on September 5, 2022:

    Thx, done

  14. DrahtBot removed the label Needs rebase on Sep 1, 2022
  15. ryanofsky approved
  16. ryanofsky commented at 5:06 PM on September 1, 2022: contributor

    Code review ACK fa67d7cafccb88b508e4eecdd9d66c2582647851. Left some minor suggestions, not important

  17. in src/univalue/include/univalue.h:54 in fa1ce3ab42 outdated
      51 |      void setInt(uint64_t val);
      52 |      void setInt(int64_t val);
      53 |      void setInt(int val_) { return setInt(int64_t{val_}); }
      54 |      void setFloat(double val);
      55 | -    void setStr(const std::string& val);
      56 | +    void setStr(std::string str);
    


    ryanofsky commented at 5:09 PM on September 1, 2022:

    In commit "univalue: Avoid std::string copies" (fa1ce3ab42fd5fd761b934b8db9fbf9f78bde842)

    Not sure if this was intended, but this is breaking the pattern of calling all setXXX arguments val


    maflcko commented at 12:22 PM on September 5, 2022:

    Yes this is intended. Note that the argument are named inconsistently val, and val_. Happy to change to something other than str, maybe val_in? Though, I think the args should be named the same in the header file as they are in the cpp file.

  18. ryanofsky approved
  19. univalue: Avoid std::string copies 1111c7e3f1
  20. maflcko force-pushed on Sep 5, 2022
  21. maflcko force-pushed on Sep 5, 2022
  22. aureleoules approved
  23. aureleoules commented at 4:28 PM on November 3, 2022: member

    ACK fa61fd688fac7bc49844c8afef9f2116c7ac71dd

  24. univalue: string_view test fa09525751
  25. maflcko force-pushed on Nov 7, 2022
  26. aureleoules approved
  27. aureleoules commented at 9:01 AM on November 7, 2022: member

    reACK fa095257511e53d7a593c6714724aafb484e6b6f

  28. maflcko requested review from ryanofsky on Nov 7, 2022
  29. ryanofsky approved
  30. ryanofsky commented at 5:21 PM on November 13, 2022: contributor

    Code review ACK fa095257511e53d7a593c6714724aafb484e6b6f

  31. martinus commented at 6:36 AM on November 14, 2022: contributor
  32. martinus approved
  33. maflcko merged this on Nov 14, 2022
  34. maflcko closed this on Nov 14, 2022

  35. maflcko deleted the branch on Nov 14, 2022
  36. sidhujag referenced this in commit ff49ec4b07 on Nov 14, 2022
  37. bitcoin locked this on Nov 14, 2023

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: 2026-05-20 06:53 UTC