-
Notifications
You must be signed in to change notification settings - Fork 490
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
we reserve the right to reduce our amount of UB #1397
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused, this now says "we reserve the right to [...] guarantee that it will always be undefined"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Josh is trying to capture the point that Niko raised below: We may want to promise that certain operations will always be UB, so that unsafe code can rely on that. edit: I don't personally think it's necessary to say this, but perhaps it would be good to say, ideally in another sentence to reduce confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the point of saying "in the future we may want to promise that something will always be UB". On its own, how is that statement useful for anyone? I would understand the point of marking particular items in the UB list as "this will definitely always be UB", but we don't currently have any such items, or do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung The wording could absolutely change. The wording currently in the PR says that we could define it in the future. The point Niko made, which I wanted to capture, was that we could also commit to not defining something (or for that matter commit to not defining it in particular ways), which is something that some crates may need us to do.
I agree that that's already implicit, but at the same time, someone reading this might want to rely on a guarantee and know we won't define the behavior unexpectedly in a way that breaks their code, so they could ask for it to be guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I still don't understand whom this comment is serving/helping. People will be asking for us to commit to something like that no matter what we write here. But sure I can try to put in a phrase along those lines. I just worry it will cause a lot more confusion than it will help people. It would certainly confuse the heck out of me without a multi-paragraph explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my hand at it, but honestly I don't like it. I think the text would be semantically equivalent but less confusing without that new sentence.