streams: Fix broken streams_vector_reader test. Remove unused seek(size_t). #14357

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:vectorreader-seek-n-with-negative-n changing 3 files +9 −22
  1. practicalswift commented at 2:22 PM on September 30, 2018: contributor

    Fix broken streams_vector_reader test. Remove unused seek(size_t).

    Before this change the test streams_vector_reader triggered an unintended unsigned integer wraparound. It tried so seek using a negative value in reader.seek(-6).

    Changes in this PR:

    • Fix broken VectorReader::seek(size_t) test case
    • Remove unused seek(size_t)
  2. practicalswift force-pushed on Sep 30, 2018
  3. practicalswift commented at 11:18 AM on October 1, 2018: contributor

    Friendly ping @jimpo – VectorReader was introduced in 947133dec92cd25ec2b3358c09b8614ba6fb40d4 and the test was added in 87f2d9ee43a9220076b1959d1ca65245d9591be9 :-)

    Should VectorReader::seek have SEEK_CUR or SEEK_SET behaviour? :-)

  4. practicalswift renamed this:
    Add support for negative relative seeks in VectorReader::seek (already assumed by tests)
    streams: Fix broken VectorReader::seek(n) + streams_vector_reader test
    on Oct 1, 2018
  5. practicalswift renamed this:
    streams: Fix broken VectorReader::seek(n) + streams_vector_reader test
    streams: Fix broken VectorReader::seek(n) and/or streams_vector_reader test
    on Oct 1, 2018
  6. practicalswift commented at 9:12 AM on October 2, 2018: contributor

    @donaloconnor Agreed! It could be that @jimpo meant to give seek absolute seek (SEEK_SET) behaviour (alternative 2 in the PR description) and the -6 (instead of 0) in the test is just a typo. With that solution the switch to signed integer is not needed :-)

    I'll await some clarification on the intention here and adjust accordingly.

  7. donaloconnor commented at 9:18 AM on October 2, 2018: contributor

    Sorry I didn't mean to submit that. :-) just the first thing I spotted.

  8. in src/test/streams_tests.cpp:106 in b126ac0336 outdated 
     100 | @@ -101,8 +101,17 @@ BOOST_AUTO_TEST_CASE(streams_vector_reader)
 101 |      signed int d;
 102 |      BOOST_CHECK_THROW(reader >> d, std::ios_base::failure);
 103 |  
 104 | +    BOOST_CHECK(reader.empty());
 105 | +    BOOST_CHECK_THROW(reader.seek(-7), std::ios_base::failure);
 106 | +    BOOST_CHECK(reader.empty());
    jimpo commented at 12:09 AM on October 3, 2018:

    This is redundant with the BOOST_CHECK_EQUAL(reader.size(), 0) two lines down. I think this one can be removed.

  9. in src/test/streams_tests.cpp:114 in b126ac0336 outdated 
     109 | +    BOOST_CHECK_EQUAL(reader.size(), 0);
 110 |      reader.seek(-6);
 111 | +    BOOST_CHECK_EQUAL(reader.size(), 6);
 112 | +    BOOST_CHECK_THROW(reader.seek(-1), std::ios_base::failure);
 113 | +    BOOST_CHECK_EQUAL(reader.size(), 6);
 114 | +    BOOST_CHECK(!reader.empty());
    jimpo commented at 12:11 AM on October 3, 2018:

    I don't think assertion is that useful after the one above. Correct behavior of empty() is sufficiently covered elsewhere in this test IMO.

  10. jimpo commented at 12:12 AM on October 3, 2018: contributor

    Thanks, good catch. Concept ACK.

    I copied seek from CVectorWriter, which is why it's a relative seek. If changing to SEEK_SET-type behavior, I think the same should be done for CVectorWriter.

  11. practicalswift force-pushed on Oct 3, 2018
  12. practicalswift force-pushed on Oct 3, 2018
  13. practicalswift force-pushed on Oct 3, 2018
  14. practicalswift force-pushed on Oct 3, 2018
  15. practicalswift force-pushed on Oct 3, 2018
  16. practicalswift force-pushed on Oct 3, 2018
  17. practicalswift force-pushed on Oct 3, 2018
  18. practicalswift renamed this:
    streams: Fix broken VectorReader::seek(n) and/or streams_vector_reader test
    streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust.
    on Oct 3, 2018
  19. practicalswift commented at 1:20 PM on October 3, 2018: contributor

    @jimpo @donaloconnor PR updated. Please re-review :-)

  20. practicalswift renamed this:
    streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust.
    streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust. Add tests.
    on Oct 3, 2018
  21. practicalswift commented at 3:23 PM on October 3, 2018: contributor

    Turns out that seek(…) is unused outside of the tests. Removed it.

    Removed code is guaranteed to be bug free :-)

  22. practicalswift commented at 7:48 AM on October 4, 2018: contributor

    @jimpo Would you mind re-reviewing? :-)

  23. in src/test/streams_tests.cpp:105 in 91f1848ce3 outdated 
     101 | @@ -102,15 +102,15 @@ BOOST_AUTO_TEST_CASE(streams_vector_reader)
 102 |      BOOST_CHECK_THROW(reader >> d, std::ios_base::failure);
 103 |  
 104 |      // Read a 4 bytes as a signed int from the beginning of the buffer.
 105 | -    reader.seek(-6);
 106 | -    reader >> d;
 107 | +    VectorReader newReader(SER_NETWORK, INIT_PROTO_VERSION, vch, 0);
    jimpo commented at 8:12 AM on October 4, 2018:

    nit: new_reader as per style guide. Or reassign reader = VectorReader(SER_NETWORK, INIT_PROTO_VERSION, vch, 0)

  24. jimpo commented at 8:13 AM on October 4, 2018: contributor

    utACK 91f1848

  25. donaloconnor commented at 3:33 PM on October 6, 2018: contributor

    Sorry I just realized I missed the new commit removing the seeks.

    utACK https://github.com/bitcoin/bitcoin/commit/91f1848ce3e15a9944b7cdd7bb7c52c2f7551211 :+1:

  26. practicalswift force-pushed on Oct 7, 2018
  27. practicalswift renamed this:
    streams: Fix broken incorrect streams_vector_reader test. Make VectorReader::seek(size_t) more robust. Add tests.
    streams: Fix broken streams_vector_reader test. Remove unused seek(size_t).
    on Oct 7, 2018
  28. practicalswift commented at 11:49 AM on October 7, 2018: contributor

    @jimpo @donaloconnor Feedback addressed. Please re-review :-)

  29. donaloconnor commented at 1:09 PM on October 7, 2018: contributor

    utACK ff7d6551feac15b60852b9d6553b54310800ccb6

  30. practicalswift force-pushed on Nov 6, 2018
  31. practicalswift commented at 11:28 PM on November 6, 2018: contributor

    Added commit 078155c8a2cc5eef5b92403e3d17c731d27b45d7 which removes UBSan suppression. Please re-review :-)

  32. jimpo commented at 5:44 AM on November 8, 2018: contributor

    utACK 078155c8a2cc5eef5b92403e3d17c731d27b45d7

  33. MarcoFalke commented at 6:04 PM on November 8, 2018: member

    utACK 078155c

  34. DrahtBot added the label Needs rebase on Nov 23, 2018
  35. streams: Remove unused seek(size_t) 958e1a307e
  36. Remove UBSan suppression 4f4993fe2a
  37. practicalswift force-pushed on Nov 23, 2018
  38. practicalswift commented at 3:59 PM on November 23, 2018: contributor

    Rebased!

  39. DrahtBot removed the label Needs rebase on Nov 23, 2018
  40. MarcoFalke commented at 9:28 PM on November 26, 2018: member

    re-utACK 4f4993fe2ac25cb2676d35a98e2b22da1add1bb1

  41. practicalswift commented at 9:02 PM on December 28, 2018: contributor

    @jimpo @donaloconnor @MeshCollider Would you mind reviewing/re-reviewing? :-)

  42. donaloconnor approved
  43. donaloconnor commented at 12:01 PM on December 31, 2018: contributor

    utACK - lgtm

  44. MarcoFalke referenced this in commit 88bbcdc4e9 on Jan 5, 2019
  45. MarcoFalke merged this on Jan 5, 2019
  46. MarcoFalke closed this on Jan 5, 2019
  47. schancel referenced this in commit 6c25e0941f on Apr 19, 2019
  48. proteanx referenced this in commit 6cdc3c0673 on Apr 26, 2019
  49. practicalswift deleted the branch on Apr 10, 2021
  50. Munkybooty referenced this in commit 6cc4dbaf77 on Aug 17, 2021
  51. Munkybooty referenced this in commit 2eedb34e6f on Aug 20, 2021
  52. Munkybooty referenced this in commit 6c26fc8a9d on Sep 5, 2021
  53. Munkybooty referenced this in commit 1dff133605 on Sep 5, 2021
  54. Munkybooty referenced this in commit d96e4f1d79 on Sep 7, 2021
  55. Munkybooty referenced this in commit aefeef25b8 on Sep 7, 2021
  56. gades referenced this in commit 5a0e4de5d7 on Apr 29, 2022
  57. DrahtBot locked this on Aug 18, 2022
Contributors
practicalswiftdonaloconnorjimpoMarcoFalke
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-04-16 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me