-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Make Allocation::bytes private #63561
Merged
Merged
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
9b9eecf
Store allocation size, make bytes, undef_mask private
HeroicKatora 2228b3f
Derive HashStable for Allocation
HeroicKatora 98cff69
Move copy of undef_mask into allocation
HeroicKatora d8c5bc7
Replace usage of alloc.bytes in interpret
HeroicKatora 7b941e3
Expose encapsulated undef mask as immutable
HeroicKatora df58fcf
Fix codegen with explicit allocation byte access
HeroicKatora 85d6b7b
Address naming and comments from reviews
HeroicKatora 6fe31fe
Make allocation relocation field private
HeroicKatora bee2d37
Move relocation range copies into allocation
HeroicKatora 7388cb4
Fixup remaining direct relocation field references
HeroicKatora 823c3b9
Improve documentation around allocation accessors
HeroicKatora f3c435e
Reorder AllocationDefinedness members
HeroicKatora 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
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
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
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
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
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
Oops, something went wrong.
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.
Can you make this private and provide a read-only accessor, similar to what you did for
undef_mask
?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 already exists a method
relocations
, which returns relocations within a range specified byptr
andsize
, so I'd name the corresponding accessorall_relocations
instead.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.
Also,
interpret/memory.rs
tries to access it directly. I'd move code in a similar manner as compressed undef mask.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.
Sounds good
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'm not available until Wednesday next week but the rest should be ready.
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.
take all the time you need
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.
The name
fn relocations
for the accessor is already taken:rust/src/librustc/mir/interpret/allocation.rs
Lines 424 to 430 in 85ed538
I'd rename the existing method to
get_relocations
which would be aligned withget_bytes
,check_bytes
,check_relocations
to be concerned with interpreter execution.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.
Sounds good.
Makes me wonder who will even need the general read-only general accessor. Maybe that should also be
raw_relocations
or so, to make it less appealing to call?