Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix minimum Duration value #1385

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

danwilliams
Copy link
Contributor

@danwilliams danwilliams commented Jan 26, 2024

Summary of changes

Note: See the Update section at the end for the final position on this PR after discussions.

This PR fixes a bug found in the representation and treatment of the minimum Duration value, as explained in detail below.

It will be best to review the changes per-commit, as the first commit adds tests and the second commit adds the fix. The second commit is very small.

The problem

The documentation clearly states in multiple locations that the possible range for a Duration is -i64::MAX through to +i64::MAX milliseconds (see References below). However, in testing it was found that the minimum value is inconsistently supported.

When creating a new Duration using Duration::milliseconds(), it is possible to create values using the full range of an i64, and create a Duration at the absolute limit of i64::MIN, which is of course not quite the same as -i64::MAX, but one millisecond below it.

However, when adding two Duration instances together, then even if the result would be within the limits of an i64, an overflow error occurs if the result falls within the bounds of the very smallest millisecond; i.e. this operation respects -i64::MAX as the limit rather than i64::MIN.

Creating a new Duration using Duration::seconds(), or higher magnitudes, is not affected by this issue due to the difference getting rounded away.

In other words, addition is artificially limiting the minimum value by an extra millisecond when compared to what is actually possible - but constructors are allowing the full i64 range.

List of values supported:

  • Duration::nanoseconds(): i64::MIN
  • Duration::microseconds(): i64::MIN
  • Duration::milliseconds(): i64::MIN
  • Duration::seconds(): Unaffected
  • Duration::minutes(): Unaffected
  • Duration::hours(): Unaffected
  • Duration::days(): Unaffected
  • Duration::weeks(): Unaffected
  • Add -> Duration::add(): -i64::MAX
  • Duration.checked_add(): -i64::MAX
  • Sub -> Duration::sub(): -i64::MAX
  • Duration.checked_sub(): -i64::MAX

The cause

The discrepancy was traced to the definition of duration::MIN, available through Duration::min_value(), which specifies:

pub(crate) const MIN: Duration = Duration {
    secs: -i64::MAX / MILLIS_PER_SEC - 1,
    nanos: NANOS_PER_SEC + (-i64::MAX % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
};

Meanwhile, creation through the various constructor functions follows two different approaches: those of a magnitude of seconds and above check MIN (and MAX), whereas those of a magnitude of milliseconds and below check against the range of the i64 type.

Seconds and above use:

pub fn try_seconds(seconds: i64) -> Option<Duration> {
    let d = Duration { secs: seconds, nanos: 0 };
    if d < MIN || d > MAX {
        return None;
    }
    Some(d)
}

Milliseconds and below use:

pub const fn milliseconds(milliseconds: i64) -> Duration {
    let (secs, millis) = div_mod_floor_64(milliseconds, MILLIS_PER_SEC);
    let nanos = millis as i32 * NANOS_PER_MILLI;
    Duration { secs, nanos }
}

Perhaps the best illustration of this bug is that subtracting zero from a Duration of i64::MIN fails - but it fails at the checked_sub() stage, and not at the creation stage:

// This gets created...
Duration::milliseconds(i64::MIN).checked_sub(&Duration::milliseconds(0))
// ...but it fails here .........^

The solution

Which of the two approaches is correct? That is not entirely clear. However, there seems to be no good reason to constrain the minimum value by an extra millisecond, so it seems the premise of the code in the definition of MIN is erroneous - possibly due to a miscalculation when the code was originally written.

When considering MAX, the range is the maximimum possible number of milliseconds, with zero nanoseconds.

The behaviour of MIN should be the same in reverse, surely?

Also supporting this reasoning is the fact that there are no try_*() methods available for magnitudes lower than seconds, as the functions for milliseconds, microseconds, and nanoseconds all rely on the i64 type limiting the possible input. If the artificial constraint is required, then these should have try_*() methods added and should fail for values in that lowest millisecond - which seems annoying.

Furthermore, the impact onto existing code relying upon Chrono is higher by restricting those areas currently using i64::MIN to use -i64::MAX than vice versa.

Therefore, this PR addresses the issue by modifying the allowed minimum limit in MIN, thereby bringing it into line with the i64-based calculations and requiring no further changes to logic.

If this is not the right outcome then this approach can of course be changed, which will involved modifying the logic of various functions that are otherwise inconsistent, and it would be good to understand the reason for the lowest millisecond being removed from usable range in order to add appropriate documentation.

The fix

The first step was to implement a range of tests against the MAX and MIN values, and the various constructors and possible arithmetic operations, in order to ensure complete coverage and show the issue clearly before making any changes. The first commit shows this - and demonstrates the inconsistency, as some of the tests fail. These failures are noted with comments.

The next step was to amend the MIN definition to bring it in line with the full range of i64, after which the tests all pass.

Previously:

pub(crate) const MIN: Duration = Duration {
    secs: -i64::MAX / MILLIS_PER_SEC - 1,
    nanos: NANOS_PER_SEC + (-i64::MAX % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
 };

After the implementation of the fix:

pub(crate) const MIN: Duration = Duration {
    secs: i64::MIN / MILLIS_PER_SEC - 1,
    nanos: NANOS_PER_SEC + (i64::MIN % MILLIS_PER_SEC) as i32 * NANOS_PER_MILLI,
 };

Note, the change for MIN.secs is for clarity and consistency only - the new and previous expressions here are equivalent, and result in the same number being computed, due to rounding. The actual difference comes from the change to MIN.nanos - changing the modulo to use i64::MIN instead of -i64::MAX results in a modulus of 192 instead of 193, which fixes the bug and allows the full i64 range to be used for representing milliseconds, aligning with the constructor behaviour.

The documentation has also been updated to reflect the range is from i64::MIN and not -i64::MAX.

Impact

Because this fix only affects the minimum Duration, and because it furthermore only affects comparison operators and not construction, the impact should be minimal if not nil. Once the fix was implemented, the failing tests passed, and there was no further effect on any other tests. Any code using Chrono should therefore be unaffected, as the values created will be unaffected, and the only different is that some values that were previously rejected (the ones in the lowest possible millisecond) will now be accepted.

References:

In the definition for duration::MIN:

/// The minimum possible `Duration`: `-i64::MAX` milliseconds.

And in the definition for Duration::try_seconds():

/// Makes a new `Duration` with given number of seconds.
/// Returns `None` when the duration is more than `i64::MAX` milliseconds
/// or less than `-i64::MAX` milliseconds.

And again in the definition for Duration.num_milliseconds():

// A proper Duration will not overflow, because MIN and MAX are defined
// such that the range is exactly i64 milliseconds.

And finally in the definition of Duration::min_value():

/// The minimum possible `Duration`: `-i64::MAX` milliseconds.

Points of note

Tests

The tests added have been done so with the purpose of specifically identifying and explaining the issue being fixed, whilst also providing useful information for longer-term purposes. To this end, explanatory comments have been added.

Branch point

The branch point chosen has been the latest tagged release, with the PR targeting the 0.4.x branch.

Update

After discussions, it was established that use of -64::MAX as the minimum value was intentional (and implemented recently, under #1334), due to the ease of swapping sign (e.g. using abs()) without having to perform checks. Although this does incur some surprise when first encountered, it does make sense; and, providing it is documented sufficiently, is completely usable and suitable.

The fix in this PR has therefore been changed, so that the creation of milliseconds is constrained to -i64::MAX instead of adjusting MIN to i64::MIN. This satisfies the disconnect between creation and arithmetic operations.

This has been effected through the introduction of a new Duration::try_milliseconds() function, allowing a fallible check to be made. Duration::milliseconds() has been updated to use this. Unfortunately this does mean that Duration::milliseconds() can now panic, which is hugely undesirable, but this is in keeping with the rest of the Chrono constructors and so this PR follows that pattern.

Additional documentation has been added to try and add clarity to the behaviour and intention.

  - Added tests for creating the maximum and minimum allowable values of
    Durations having a magnitude of seconds, testing the limits plus one
    value beyond the limits in both directions. These tests all pass.

  - Expanded the tests for creating the maximum and minimum allowable
    values of Durations having a magnitude of milliseconds. These tests
    examine the results in more detail, document what is being tested,
    and also test one value beyond the limits in both directions.
    Notably, the test for Duration::milliseconds() construction for
    i64::MIN currently fails, as it is erroneously allowed. This test is
    ignored for now, until the fix is applied.

  - Expanded the tests for creating the maximum and minimum allowable
    values of Durations having a magnitude of microseconds and
    nanoseconds. These tests examine the results in more detail,
    document what is being tested, and also test one value beyond the
    limits in both directions. They also test the maximum reportable
    value from .num_*() and the maximum storable value of the Duration
    separately.

  - Separated out the tests for MAX and MIN, for clarity.

  - Added additional tests for addition and subtraction operations on
    Durations, ensuring that equivalent tests are performed against both
    operations, such as adding and subtracting zero, adding and
    subtracting one nanosecond, and others.

  - Added tests for greater-than and less-than comparison of two
    Durations, to ensure that internal representation of partial seconds
    is correctly ordered.
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bf70419) 91.69% compared to head (0024cee) 91.84%.
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1385      +/-   ##
==========================================
+ Coverage   91.69%   91.84%   +0.15%     
==========================================
  Files          38       38              
  Lines       17608    17491     -117     
==========================================
- Hits        16145    16064      -81     
+ Misses       1463     1427      -36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djc djc requested a review from pitdicker January 26, 2024 09:27
@djc
Copy link
Member

djc commented Jan 26, 2024

@danwilliams thanks for working on this! I quickly looked over the library changes and I think they make sense.

@pitdicker do you have time to review this? You may have more context on the desirability of such a change.

@danwilliams
Copy link
Contributor Author

@djc no worries! 🙂 I initially thought I had a bug in my code, until I traced the cause - and I knew a change like this would need very careful corroboration and indeed a full explanation of cause, effect, and impact in order to be convincing and get considered for merge.

@pitdicker I'm very open to any feedback you may have - I believe I have chosen the most appropriate fix, but let me know!

@djc
Copy link
Member

djc commented Jan 26, 2024

full explanation of cause, effect, and impact in order to be convincing and get considered for merge.

To be honest, for me your OP is not ideal: the summary doesn't really go into what the fix is or what motivated the fix, and the other sections are... a little hard to digest.

Which of the two approaches is correct? That is not entirely clear. However, there seems to be no good reason to constrain the minimum value by an extra millisecond, so it seems the premise of the code in the definition of MIN is erroneous - possibly due to a miscalculation when the code was originally written.

In practice, we've found that keeping a little margin can be useful, see #1317 and subsequent discussion in #1382.

@danwilliams
Copy link
Contributor Author

@djc well colour me confused!

the summary doesn't really go into what the fix is

There's a section called "The fix" that explains this in detail.

or what motivated the fix

There's a section called "The problem" that covers this 🙂

Basically I found in my own codebase that I was experiencing odd behaviour. My tests were showing inconsistencies, which I tracked down to Chrono.

I do understand that it takes time to read, but I've laid out the PR description and the accompanying commits in sufficient detail to make the problem clear. The problem is that Chrono currently has a bug in that the treatment of the minimum Duration value is different when creating a Duration versus performing operations on Durations.

The crux of it is what I mentioned here:

Perhaps the best illustration of this bug is that subtracting zero from a Duration of i64::MIN fails - but it fails at the checked_sub() stage, and not at the creation stage:

// This gets created...
Duration::milliseconds(i64::MIN).checked_sub(&Duration::milliseconds(0))
// ...but it fails here .........^

In short there is currently a bug, and I've chosen the least-impactful change in terms of a fix. The alternate fix would require additional logic changes and have a wider impact, but it's up to you guys. I personally don't feel Duration requires the same padding as DateTime, as it doesn't have the same cases to consider, but that's just my opinion. Also, this current bug is not a case where padding has been added purposefully, or with any beneficial effect - it's simply a bug. I feel any such padding would have to have a reason behind it? And presumably should apply to the maximum value as well as the minimum? And I don't see a motivation for that 😅

I hope that helps! ⭐

@pitdicker
Copy link
Collaborator

pitdicker commented Jan 26, 2024

@danwilliams Thank you for investigating and all the effort you already put into it!

Duration::MIN was changed to be the same as -Duration::MAX in #1334. The goal was to make Duration easier to use by removing the possibility of panics when negating or taking the absolute value of a duration. It does not really make sense for our Duration type to behave like a two-complements integer, given that its range is chosen pretty much arbitrary and is far less than its types support.

Even in just chrono this fixes potential panics in something like 10 methods that happen to negate a duration hidden under a bunch of abstractions. I expect it is no different for user code.

But as you correctly point out there now is a bug, as Duration::milliseconds is missing the range check and has no fallible variant. I think it is best to add the range check there, similar to all other initialization methods with larger units. Would you be willing to adjust your PR to do so?

@danwilliams
Copy link
Contributor Author

@pitdicker aha, thanks for the explanation! Perhaps I should have looked a bit harder for a cause, but it just seemed like a mistake 😅

That also explains now some of the confusion I've had in encountering this, because I had some code that was partly-done which seemed okay, and then I stumbled upon this. Unfortunately I didn't put two and two together and realise that it was affected by the 0.4.32 version of Chrono that I had just upgraded to, and although I had scanned the changelog, that was before I found this problem and therefore I missed the connection. That makes a lot of sense now!

I'm happy to update my PR to implement the "other" option, which is the one you mention - i.e. preserving use of -i64::MAX and updating various locations elsewhere to respect this. Your reasoning works for me - I don't think use of i64::MIN is bad, but as you reason under #1334, it is arbitrary, and therefore there is some nicety in being able to reverse the sign without concern. I think that people will need to perform checks either way, and so in some ways it's much of a muchness. The avoidance of issues when using abs() is compelling, though 👍

I will add the changes to this PR later today. Please note, though, that they will have a wider impact than the "smaller" fix because use of -i64::MAX not only requires more locations to be changed but also has the potential to affect existing codebases in the wider community, as it will no longer accept that particular value for creation. I don't know if that was considered for #1334, but I doubt it matters very much in reality.

In terms of the required changes, I feel there are different levels I could go to:

  1. Just fix the bug - this will require adding the range checks to the constructor functions that omit them
  2. Also add in equivalent try_*() methods for those constructors that will now be able to fail

I'm happy to cover both, but I am mindful of keeping PRs focused. My suggestion would be that I cover the first level under this PR, to get the bug fixed, and then raise a second PR to then add in the additional checking functions? Will that be okay with you? 🙂

Also, thank you for reviewing so quickly! ❤️

@djc
Copy link
Member

djc commented Jan 26, 2024

I'm happy to cover both, but I am mindful of keeping PRs focused. My suggestion would be that I cover the first level under this PR, to get the bug fixed, and then raise a second PR to then add in the additional checking functions? Will that be okay with you? 🙂

Sounds great, thanks!

@pitdicker
Copy link
Collaborator

pitdicker commented Jan 26, 2024

Thank you. It is only since 0.4.30 that chrono has a completely standalone Duration type, independent of the time 0.1 crate. 0.4.32 was the first release with serious changes to make the methods on that type less panic-prone.

I think the easiest route is to add Duration:try_milliseconds, and then use that in Duration::milliseconds. And maybe make some adjustments to the comments that still claim we match the exact range of i64 milliseconds?

  - Added Panics and Errors sections where appropriate, as these are
    generally-expected and help draw attention to the fact that the
    standard (i.e. non-try) constructors can panic. The Errors section
    for the try constructors is common practice when returning None for
    overflow situations as well as for functions actually returning a
    Result.

  - Added an further explanation of the behaviour of the seconds()
    constructor.

  - Minor additional readability edits.
  - Added a new Duration::try_milliseconds() function, to attempt to
    create a new milliseconds-based Duration, and return None if it
    fails, based on checking against MAX and MIN. Currently, failure can
    only happen for exactly one value, which is i64::MIN.

  - Updated Duration::milliseconds() to call try_milliseconds() and
    panic if None is returned. Although panicking in production code and
    especially library code is bad, this is in keeping with current
    Chrono behaviour. Note that this function is now no longer const.

  - Updated the Duration::milliseconds() documentation to make it clear
    that it now panics.

  - Added documentation to Duration::microseconds() and nanoseconds() to
    make it clear that they are infallible.

  - All tests now pass, including the one previously ignored.
danwilliams added a commit to danwilliams/chrono that referenced this pull request Jan 27, 2024
This reverts commit 3417668.

This has been reverted due to discussion on the PR, where the use of the
-i64::MAX value was explained. The fix will be implemented in a
different manner.

chronotope#1385
  - Added a description of internal storage and permissable range to the
    Duration type.

  - Adjusted formatting of `Duration` in a number of related function
    Rustdoc blocks, to aid readability.

  - Clarified comments for Duration::num_milliseconds().
@danwilliams
Copy link
Contributor Author

@pitdicker gotcha! That makes sense, and I've gone ahead on that basis 🙂

I think the easiest route is to add Duration:try_milliseconds, and then use that in Duration::milliseconds.

I have done exactly that. I reverted my original fix, adjusted the tests to align with your explanation, and added Duration::try_milliseconds() in the style used elsewhere. I have indeed updated Duration::milliseconds() to use Duration::try_milliseconds(), but it does pain me somewhat to be introducing a panic... 😅 Although it makes me wince slightly, I've justified it as being in line with existing behaviour, and hopefully will be properly corrected in v0.5 where you can make more extensive changes 🤞 I do think I will need to offer up a sacrifice to the coding gods of Rust later to appease them though! 😂

And maybe make some adjustments to the comments that still claim we match the exact range of i64 milliseconds?

I've gone through and made adjustments, ensuring that the descriptions are accurate and explanatory, and I've also taken the liberty of adding the expected-as-standard sections for "Errors" and "Panics" where appropriate, to draw attention to these cases. I have also added a short description and explanation to the Duration type itself. I haven't wanted to go too far, as this is a fix - so I've tried to keep it all in scope; but please let me know if I've overstepped.

I've also added an "Update" section to the PR description to make it clear as to the outcome.

Hopefully this is all okay - please let me know if you would like any changes? 🙂

@pitdicker
Copy link
Collaborator

Thank you! I'll have time to look at it later today, and like your doc changes and the change to milliseconds and try_milliseconds.
Can you please rework the commits to remove your earlier approach?

The tests seem a bit excessive, but I'll have to read them a bit better first 😄.

@danwilliams
Copy link
Contributor Author

danwilliams commented Jan 27, 2024

@pitdicker No worries! I'm glad you like the documentation changes 👍

Can you please rework the commits to remove your earlier approach?

Hmmm... I kept them in simply because when you guys merge, you squash, and therefore those commits will disappear. So the final result will have the outcome only, and meanwhile I felt that retaining them gives clarity to what has happened over the course of the PR, and how the update has changed things from the original fix I proposed. I'd prefer to keep them here for that purpose only, as they align?

The tests seem a bit excessive

Quite possibly the accompanying comments are a little verbose 😊 However, I added those for clarity over the behaviour here, as anyone looking into these ranges in future will benefit from knowing the intent. Each actual test is chosen carefully in order to ensure sensible things are being tested. Arguably the ones for Ord could be reduced a little... let me know!

@pitdicker
Copy link
Collaborator

Hmmm... I kept them in simply because when you guys merge, you squash, and therefore those commits will disappear.

Well, I might do so but your are not going to get @djc's approval without reworking the commits. Sorry. And we don't squash, your commits will not end up to be useless.

@danwilliams
Copy link
Contributor Author

@pitdicker Interesting - I didn't see any/many branch merge points, but upon closer inspection that might be because you're using rebase merge - that's unclear.

Anyway, thank you for the explanation - that makes sense 👍 I'll rebase and pop up a changed version now 🙂

@pitdicker
Copy link
Collaborator

Thank you! Yes, we use rebase and merge with a small number of exceptions.

@danwilliams
Copy link
Contributor Author

@pitdicker All updated - I removed the commits that applied and reverted the fix, and merged the commit amending the test changes with the original commit where I added the additional tests. I also reworked the commit message to be accurate now that it contains the sum of both actions.

Given these commits will all come into the main branch, this should form a clearer narrative of the ultimate outcome, whilst retaining atomicity of changes. Thanks again for your clarification on your process in this regard 👍

Does this seem okay now, or would you like any further changes? 🙂

Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work (better than what I do 😆)!

I only have one request.

src/duration.rs Show resolved Hide resolved
@danwilliams
Copy link
Contributor Author

@pitdicker thanks again for your time reviewing this - and for the unexpected compliment! 😊

Further to your feedback request I have tweaked the failing test to be ignored initially, until the fix is applied, thereby ensuring each commit still passes all tests. I hope that's okay 🤞

Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! @djc will have the final say.

danwilliams added a commit to danwilliams/rubedo that referenced this pull request Jan 28, 2024
  - Adjusted MIN_MILLISECONDS, MIN_NANOSECONDS_FULL, and
    MIN_MICROSECONDS_FULL due to changes in Chrono 0.4.32 that were not
    previously noticed, which restrict the minimum value of milliseconds
    by 1, by changing the limit from i64::MIN to -i64::MAX. This was
    carried out under chronotope/chrono#1334.
    Notably, Chrono 0.4.32 does still allow creation of a Duration with
    a milliseconds value of i64::MIN, which is why this change was not
    noticed sooner. This will be corrected when Chrono 0.4.34 is
    released, with chronotope/chrono#1385. In
    the meantime, the correction of constant values here means that we
    are now compatible with both versions and also do not suffer from
    the current Chrono bug.
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good modulo a few nits.

src/duration.rs Outdated
Comment on lines 236 to 247
// Technically we don't need to compare against MAX, as this function
// accepts an i64, and MAX is aligned to i64::MAX milliseconds. However,
// it is good practice to check it here in order to catch any future
// changes that might be made.
if d < MIN || d > MAX {
return None;
}
Some(d)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should test against MAX here. Instead we should have a test that catches this case, shifting the burden from run time to development time. (Keeping a comment around about why we don't need to test against MAX here is useful, though.)

I'd also prefer to spell this

match d < MIN {
    true => None,
    false => Some(d),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm. I see a failure to check against MAX in the code would be poor in case MAX is changed in future. I am not clear on how a test would necessarily catch this, as there would likely be a self-dependency and it would definitely obfuscate?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also fine not having a test, but I don't think it makes to sense to reassert things that the type system has already checked for us. (I also think it is very unlikely MAX will change.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...sorry, I overlooked replying to the style of the match you have proposed. I believe a simple if is more idiomatic here, and the match is less clear. Additionally, the short-circuit fits with the same logic elsewhere, making the similarity obvious. I'd like to keep that code as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm. I see a failure to check against MAX in the code would be poor in case MAX is changed in future.

If we change MAX we have to look at the entire module anyway (and possibly code outside it). I do have some experience in how it is easy to forget a check 😉. But I also think it is very unlikely for MAX to change.

Copy link
Member

@djc djc Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...sorry, I overlooked replying to the style of the match you have proposed. I believe a simple if is more idiomatic here, and the match is less clear. Additionally, the short-circuit fits with the same logic elsewhere, making the similarity obvious. I'd like to keep that code as-is.

Sorry, but I stand by my earlier comment. In cases like these, having the match express the last expression in one IMO makes the code easier to grok compared to having an early return path, and it lays out the two return paths right next to each other instead of them being separated both by more vertical space and offset by one horizontal tab stop. The surrounding code is probably older and hasn't gotten recent review so I don't think it's a great guide as to what makes for good style.

Copy link
Contributor Author

@danwilliams danwilliams Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind removing the check for MAX, having explained why I think it's a good idea to retain it. The point I was making is that the type system having checked it for us is coincidentally correct, and not infallibly correct, which is why usually codebase-specific limits would be carefully checked. But given the context here, I agree that it is unlikely to change, and if it does, is likely to be picked up. So although I think the change is not ideal, I don't see it as particularly harmful - done! 🙂 Pushed 👍

src/duration.rs Outdated
}

/// Makes a new `Duration` with the given number of microseconds.
///
/// The number of microseconds acceptable by this constructor is far less
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to change "far less" to just "less", since I think the "far" provides unnecessar(il)y (opinionated) details (same for the next one)/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair...? I can't say I mind either way, but "far less" is accurate and not particularly opinionated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO in this context it is irrelevant how much less it is, so better to leave it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just musing on this - it's a million times less; that does seem "far less" 😄 It's not like -i64::MAX vs i64::MIN.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not remarking on the accuracy but on the relevancy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a general feeling I have with documentation:
Documentation should not look like a cigarette pack by listing all the ways in which it is harmful. It should above all show how to solve problems, and give details and warnings as appropriate.

In the last few releases of chrono I added documentation to a lot of methods around panic cases and errors. I have not compensated it enough with positive documentation 😄.

While reviewing my though about this documentation was: "This function is infallible" is enough, but this is okay.

Ready to finish up with the requests by @djc? Then I'll rebase #1337 on your work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough 🙂 I don't see it as an issue, but equally I don't really care either way on this one. I'll push up a change for it.

@pitdicker pitdicker merged commit e031ffb into chronotope:main Feb 1, 2024
37 checks passed
@pitdicker
Copy link
Collaborator

Thank you for working on this!

@danwilliams
Copy link
Contributor Author

@pitdicker no worries! I've enjoyed getting involved ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants