serge-sans-paille / frozen

a header-only, constexpr alternative to gperf for C++14 users
Apache License 2.0
1.27k stars 103 forks source link

Standard implementation for lower_bound/upper_bound/equal_range. #125

Closed nbrachet closed 1 year ago

nbrachet commented 3 years ago

Namely (quoting cppreference.com):

adamshapiro0 commented 3 years ago

@nbrachet this is great timing - we just ran into the same issue and I was about to make this change myself when I found your PR. I suggest adding some improved test coverage as follows:

  auto lower_bound = ze_map.lower_bound(0);
  REQUIRE(lower_bound == ze_map.begin());

  lower_bound = ze_map.lower_bound(5);
  REQUIRE(lower_bound == ze_map.begin());

  lower_bound = ze_map.lower_bound(20);
  REQUIRE(lower_bound == ze_map.begin() + 1);

  lower_bound = ze_map.lower_bound(25);
  REQUIRE(lower_bound == ze_map.begin() + 2);

  lower_bound = ze_map.lower_bound(30);
  REQUIRE(lower_bound == ze_map.begin() + 2);

  lower_bound = ze_map.lower_bound(40);
  REQUIRE(lower_bound == ze_map.end());

  auto upper_bound = ze_map.upper_bound(0);
  REQUIRE(upper_bound == ze_map.begin());

  upper_bound = ze_map.upper_bound(10);
  REQUIRE(upper_bound == ze_map.begin() + 1);

  upper_bound = ze_map.upper_bound(15);
  REQUIRE(upper_bound == ze_map.begin() + 1);

  upper_bound = ze_map.upper_bound(30);
  REQUIRE(upper_bound == ze_map.end());

  upper_bound = ze_map.upper_bound(40);
  REQUIRE(upper_bound == ze_map.end());
adamshapiro0 commented 3 years ago

Oh and actually one more test change to test for the at() fix:

   SECTION("Lookup failure") {
+    REQUIRE(frozen_map.find(3) == frozen_map.end());
+    REQUIRE_THROWS_AS(frozen_map.at(3), std::out_of_range);
     REQUIRE(frozen_map.find(5) == frozen_map.end());
     REQUIRE_THROWS_AS(frozen_map.at(5), std::out_of_range);
   }
nbrachet commented 3 years ago

Thanks for the suggestions. Added in https://github.com/serge-sans-paille/frozen/pull/125/commits/c445fef5a4c8422a521575c7ee3aa54727322edf

serge-sans-paille commented 3 years ago

Thanks for this fix! Can you rebase it against master branch?

serge-sans-paille commented 1 year ago

Commited as of #148