fix split for negative whole day deltas

[Schischeck]

old version (not what you would expect):
auto [day, mam] = split_day_mam(day_idx_t{10}, delta_t{-1440});
EXPECT_EQ(day, day_idx_t{8});
EXPECT_EQ(mam.count(), 1440);

now:
auto [day, mam] = split_day_mam(day_idx_t{10}, delta_t{-1440});
EXPECT_EQ(day, day_idx_t{9});
EXPECT_EQ(mam.count(), 0);

[felixguendling]

@mority any doubts?

[mority]

@mority any doubts?

I wonder if it would be easier to convert base to minutes first, add the $\delta$ (i.e., x) and then split into days/minutes.

  auto const minutes = base.v_ * 1440 + x;
  return {day_idx_t{minutes / 1440}, minutes_after_midnight_t{minutes % 1440}};

Seems to do the job while being more readable.

[felixguendling]

Isn't this what we do for the positive case? For the negative case we need something different.

[mority]

Isn't this what we do for the positive case? For the negative case we need something different.

Sort of. For the positive case, we right now convert the $\delta$ into days and add it to base. We use modulo to get the mam.

return {static_cast<day_idx_t>(static_cast<int>(to_idx(base)) + x / 1440), minutes_after_midnight_t{x % 1440}};

For a negative $\delta$, we right now calculate a day offset t and mam value min.

auto const t = -x / 1440 + 1;
auto const min = x + (t * 1440);

So, when looking at the units, we convert x into days to get offset t. Which we then convert back into minutes in the next line.

However, we do not need two cases, if we convert into the atomic unit of minutes first. base.v_ * 1440, we then add our $\delta$ (or subtract if it is negative ) base.v_ * 1440 + x. The result is a point in time in minutes since the beginning of the timetable. Which we then split into day index and mam:

return {day_idx_t{minutes / 1440}, minutes_after_midnight_t{minutes % 1440}}

[Schischeck]

@mority your version seems to work.
Only if day_idx_t would become negative, the behavior is different. Your version would fail the following test.
The question is, of course, whether this behavior(test) makes sense or whether the value delta_t should never represent a smaller time than the start of the timetable.

 {
    auto [day, mam] = split_day_mam(day_idx_t{0}, delta_t{-1});
    EXPECT_EQ(day, day_idx_t{-1});
    EXPECT_EQ(static_cast<int16_t>(day.v_), -1);
    EXPECT_NE(static_cast<int32_t>(day.v_), -1);
    EXPECT_EQ(mam.count(), 1439);
  }

[mority]

Yeah, I think all of our solutions so far, underflow for a low enough negative $\delta$, right?

Edit: For clarification day_idx_t is an unsigned value.

[felixguendling]

Yes, timestamps outside the internal timetable interval don't have to be representable. We already subtract 5 days from the user provided timetable begin. That should be enough for all use cases that come to my mind.

[Schischeck]

Yeah, I think all of our solutions so far, underflow for a low enough negative δ , right?

Edit: For clarification day_idx_t is an unsigned value.

Yes, i just wanted to point out that they "underflow" in a different way. ;-)

[mority]

Yeah, I think all of our solutions so far, underflow for a low enough negative δ , right?
Edit: For clarification day_idx_t is an unsigned value.

Yes, i just wanted to point out that they "underflow" in a different way. ;-)

Alright, just wanted to make sure that I did not miss anything.

[felixguendling]

Ok, then maybe the best option is to have this approach for both cases.

[felixguendling]

Thank you! :)