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

minor cleanup of pmap related files #15870

Merged
merged 1 commit into from
Apr 14, 2016
Merged

minor cleanup of pmap related files #15870

merged 1 commit into from
Apr 14, 2016

Conversation

amitmurthy
Copy link
Contributor

Based on comments in #15409


@assert min_batch_count > 0
@assert max_batch_size > 1
if max_batch_size < 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samoconnor , max_batch_size of 1 should be allowed, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

My intention was to disallow max_batch_size=1.
The rationale is that anyone who is trying to iterate over batches of 1 should probably just iterate over the collection directly. Or, put differently, anyone who sets max_batch_size=x, and x ends up being 1 probably has a bug, and almost certainly will get sub-optimal performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as 1 works correctly, I think it must be allowed. While currently it is not used anywhere else, in general, the values may be runtime configurable in a user program - for example, a config file or the environment. In such cases it is better to support all correctly computable values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind if you change this. I'll try to explain my point of view as best I can and leave it at that...
Disclaimer: My precondition design choices are influenced by being beaten by the Eiffel/DBC stick at a young age.

My feeling is that preconditions should protect against:

  • input that is invalid -- because that will stop the implementation from crashing or computing an incorrect result.
  • input that is not sane/useful -- because alerting the user early in the development process that they are doing something that makes no sense might prompt them to understand things better and write better code

I also feel that when in doubt it is better to make preconditions tighter rather than looser because:

  • it reduces the number of cases that need to be covered in testing
  • it reduces the metal load on code reviewers (e.g. considering, "will it still work in the corner case of max=1?")
  • if someone one day finds a good reason to make the precondition looser, that's easy to do without breaking anyone else's code (making a precondition tighter can break code).
  • it provides a clear indication to the user, that the author of the function intended it to be used in a certain way.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Probably a bug" is not an error. It's convenient to compare the degenerate case through the same code path with a parameter instead of having to change syntax. Real bugs can be identified via static analysis and testing.

@tkelman tkelman merged commit a056ce2 into master Apr 14, 2016
@tkelman tkelman deleted the amitm/corrections branch April 14, 2016 19:46
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.

3 participants