Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

More readable message in ArrayIndexError#3572

Merged
RazvanN7 merged 2 commits intodlang:masterfrom
nordlow:better-range-diag
Sep 29, 2021
Merged

More readable message in ArrayIndexError#3572
RazvanN7 merged 2 commits intodlang:masterfrom
nordlow:better-range-diag

Conversation

@nordlow
Copy link
Copy Markdown
Contributor

@nordlow nordlow commented Sep 24, 2021

FYI, @andralex.

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request and interest in making D better, @nordlow! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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 run digger -- build "master + druntime#3572"

@CyberShadow
Copy link
Copy Markdown
Member

Is the intention to fix the wording when the index equals the length?

@nordlow
Copy link
Copy Markdown
Contributor Author

nordlow commented Sep 24, 2021

Is the intention to fix the wording when the index equals the length?

This change certainly makes for a better interpretation in that case, but I hadn't thought of that myself actually. This change is on behalf of @andralex so safest to let him answer this question aswell.

@CyberShadow
Copy link
Copy Markdown
Member

Not to bikeshed but "too large" makes me think of an integer value that doesn't fit in a size_t.

Can we look at what terminology / phrasing other languages do, and stick to that? Maybe just "out of bounds"?

@andralex
Copy link
Copy Markdown
Member

Good point. How about

Index [7] is beyond the end of array of length 3.

Native speakers please opine!

@dkorpel
Copy link
Copy Markdown
Contributor

dkorpel commented Sep 24, 2021

This change is on behalf of @andralex so safest to let him answer this question aswell.

Where did this come up?

How about

Index [7] is beyond the end of array of length 3.

That sounds equivalent to 'exceeds', which means to "go beyond what is allowed or stipulated by (a set limit)" or "go past an allowed limit".

I'm not against changing the message, I'm just curious what's unclear about it. After I wrote the new array error messages I asked a programmer who doesn't program in D whether it was clear and he thought so. Is 'exceed' a word people don't know or don't expect in this context?

@andralex
Copy link
Copy Markdown
Member

It's not unclear, it's grammatically incorrect.

An index does not exceed an array. It exceeds the length of the array. The meaning of the message is that whatever index you give must be less than the length.

@andralex
Copy link
Copy Markdown
Member

How about this - no repetitions, no akwardness:

Index 7 out of bounds for array of length 3.

Worx?

@dkorpel
Copy link
Copy Markdown
Contributor

dkorpel commented Sep 24, 2021

An index does not exceed an array. It exceeds the length of the array.

Ah, makes sense. I originally wrote "index [20] exceeds array length 20" but I added the 'of' because it looks weird that 20 exceeds 20.

Index 7 out of bounds for array of length 3.

Sounds good

@andralex
Copy link
Copy Markdown
Member

@dkorpel thanks, and also thank you for putting in the actual work!

@moon-chilled
Copy link
Copy Markdown

moon-chilled commented Sep 24, 2021

SBCL uses: 'Invalid index 7, should be a non-negative integer below 3'. I like that because it makes it clear what's happening when the index is the array length.

@andralex
Copy link
Copy Markdown
Member

love it!

@dkorpel
Copy link
Copy Markdown
Contributor

dkorpel commented Sep 24, 2021

'non-negative' is redundant in D's case, since array indices have the unsigned size_t type.

@thewilsonator
Copy link
Copy Markdown
Contributor

Index 7 out of bounds for array of length 3.

This leads to Index 3 out of bounds for array of length 3 for an off-by-one, potentially quite confusing for a newb.

How about "Index 7 is out of bounds of range [0 .. 2]"?

@dkorpel
Copy link
Copy Markdown
Contributor

dkorpel commented Sep 24, 2021

How about "Index 7 is out of bounds of range [0 .. 2]"?

Then arr[0..3][3] would result in "Index 3 is out of bounds of range [0 .. 2]"

@nordlow
Copy link
Copy Markdown
Contributor Author

nordlow commented Sep 24, 2021

I'm gonna wait and see what fighter that still stands after a couple of more rounds... ;)

@RazvanN7
Copy link
Copy Markdown
Contributor

"Index %s is out of bounds for array of length %s" is fine with me.

Copy link
Copy Markdown
Contributor

@dkorpel dkorpel left a comment

Choose a reason for hiding this comment

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

Also update the comment on line 100:

It's essentially printf("index [%zu] exceeds array of length [%zu]", index, length)

@nordlow
Copy link
Copy Markdown
Contributor Author

nordlow commented Sep 28, 2021

Also update the comment on line 100:

It's essentially printf("index [%zu] exceeds array of length [%zu]", index, length)

Done.

@RazvanN7 RazvanN7 merged commit 7456d96 into dlang:master Sep 29, 2021
Comment thread src/core/exception.d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants