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

Error enums #22

Merged
merged 2 commits into from Feb 20, 2017
Merged

Error enums #22

merged 2 commits into from Feb 20, 2017

Conversation

marshallpierce
Copy link
Collaborator

No description provided.

@marshallpierce
Copy link
Collaborator Author

This is on top of #16.

src/lib.rs Outdated
/// Number of significant digits must be between 0 and 5
SigFigTooBig,
/// Cannot represent sigfig worth of values beyond low
CannotRepresentSigFigBeyondLow
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I expect most people will probably not match each of these variants but at least it will be nicely specific if their usage panics.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup. I think we might be able to bikeshed some better names for the variants, but it's probably not worth the time.

I do think we should give some better documentation for the variants though. This will be one of the main places where people will look for an explanation of why something didn't work, so we should make sure things are as clear as possible. For example, why can't SigFig be bigger than 5, why must high be at least twice of low, etc. For some of these we have explanations elsewhere in the code, and I think those can be moved here.

Also, a minor point, but you should enclose code things (like u64::max_value() / 2) in backticks.

pub enum AdditionError {
/// The other histogram includes values that do not fit in this histogram's range.
/// Only possible when auto resize is disabled.
OtherAddendValuesExceedRange
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Annoyingly, "addend" refers to both terms in an addition. The first one is called the "augend" but that's not what we care about. Oddly this value thing means that addition of non-resizing histograms is not commutative.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, interesting. I think that's probably fine though. If I remember correctly, this is the reason I implement only AddAssign, and not Add.

/// The other histogram includes counts that are higher than the current count for a value, and
/// counts cannot go negative. The subtraction may have been partially applied to some counts as
/// this error is returned when the first impossible subtraction is detected.
SubtrahendCountExceedsMinuendCount
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For subtraction we can have maximally precise (or pedantic) names, though!

@marshallpierce
Copy link
Collaborator Author

This addresses #10.

@jonhoo
Copy link
Collaborator

jonhoo commented Feb 20, 2017

Looking good! I'd like some better docs for the Creation errors, because I suspect people will run into them more often than we think, and that's where they'll look to get an explanation, but beyond that I'm happy.

@marshallpierce
Copy link
Collaborator Author

I took the liberty of self-bikeshedding on enum names, and also centralized the docs for the new_* methods to make it easier to maintain details on sigfigs and things. The internal documentation for the "sig figs + low = too many bits" error referred to things that aren't exposed externally (like unit_magnitude) so I opted not to just move that up to the corresponding variant.

@jonhoo jonhoo merged commit 6257531 into HdrHistogram:master Feb 20, 2017
@jonhoo
Copy link
Collaborator

jonhoo commented Feb 20, 2017

Looks good!

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.

None yet

2 participants