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

adds assert messages for container.array #7190

Merged
merged 3 commits into from
Nov 4, 2019

Conversation

burner
Copy link
Member

@burner burner commented Sep 20, 2019

No description provided.

@burner burner requested a review from PetarKirov as a code owner September 20, 2019 04:23
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @burner!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7190"

@jondegenhardt
Copy link
Contributor

jondegenhardt commented Sep 22, 2019

Thanks for doing this. These assert messages are really valuable, they have helped me many times.

The text of the assert messages could be improved by changing the text to be more consistent with the other Array asserts. This would also make them consistent with the most common assert message format used in Phobos range methods. Most valuable would be to include the data type in the assert message. In this case, "Array". Specific suggestions for the first few cases:

  • moveFront:
    • "Attempting to moveFront an empty Array"
    • "Attempting to moveFront using an out of bounds low index on an Array"
  • moveBack
    • "Attempting to moveBack an empty Array"
    • "Attempting to moveBack using an out of bounds high index on an Array"
  • moveAt
    • "Attempting to moveAt using an out of bounds index on an Array"
  • opIndex
    • "Attempting to fetch using an out of bounds index on an Array"

The style is taken from examples found by grep'ing Phobos source files for assert and Attempting.

There are many range assert messages that don't adhere to this style, but it appears to be the most common. A somewhat common difference occurs with opIndex style messages, many of which have formats <object-type> <operation> index out of bounds. Examples:

"chunks index out of bounds"
"slide slicing index out of bounds"

@jondegenhardt
Copy link
Contributor

jondegenhardt commented Sep 22, 2019

To clarify my previous comment - I'm fine with the PR being merged as is if the author doesn't want to invest additional time in further improvements to the text. This PR is a meaningful improvement over the current assert messages (none). I looked through them all and didn't see any where the text was inconsistent with the method.

A separate thing - I do wonder if the memory allocation failure cases should be asserts. Not sure what the standard mechanism is in Phobos. However, they are asserts currently, this PR is not attempting to look at that, just the assert message text.

@burner
Copy link
Member Author

burner commented Sep 24, 2019

@jondegenhardt thank you for the review. I'll keep a look later this week.

Copy link
Contributor

@jondegenhardt jondegenhardt left a comment

Choose a reason for hiding this comment

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

Very nice. Thanks for making these updates.

@burner
Copy link
Member Author

burner commented Sep 29, 2019

@jondegenhardt thanks for the review

@n8sh n8sh changed the title adds assert messages for contianer.array adds assert messages for container.array Nov 3, 2019
@burner
Copy link
Member Author

burner commented Nov 4, 2019

@n8sh @atilaneves @jondegenhardt thank you

@PetarKirov PetarKirov merged commit e70d86b into dlang:master Nov 4, 2019
@PetarKirov
Copy link
Member

(Merging manually as buildkite failure is not related to this PR.)

@burner
Copy link
Member Author

burner commented Nov 4, 2019

@PetarKirov thank you

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

Successfully merging this pull request may close these issues.

6 participants