Clarify the use of constant values #200
Conversation
|
@joncinque @ilya-bobyr Instead of adding constant, this PR improves the documentation of the "raw" (constant) values in the code. Not sure whether this is better than the changes on #191 – I slightly prefer this one since we don't need to come up with names to constants that might not clearly describe their use. |
ilya-bobyr
left a comment
ilya-bobyr
left a comment
There was a problem hiding this comment.
I like all the comments.
I think they make the code much easier to follow.
Especially for someone who does not have the whole thing already in their head.
sdk/pinocchio/src/instruction.rs
Outdated
| /// Return a pointer to a type `U` given a source pointer for `T` and the relative offset | ||
| /// to `U` in units of `T`. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller must ensure that the `source` pointer is valid for `T` and that the offset is | ||
| /// within the bounds of the memory region pointed to by `source`. The resulting pointer must | ||
| /// at an aligned memory location that can be safely cast to `*const U`. | ||
| /// | ||
| /// If any of this requirements is not valid, this function leads to undefined behavior. | ||
| #[inline(always)] | ||
| const fn offset<T, U>(ptr: *const T, offset: usize) -> *const U { | ||
| unsafe { (ptr as *const u8).add(offset) as *const U } | ||
| const unsafe fn offset<T, U>(source: *const T, offset: usize) -> *const U { |
There was a problem hiding this comment.
Unrelated to the documentation you are adding (which is great by the way), I think this function, as it is, is the ultimate foot gun :)
It removes pretty much all the constraints on the source and the destination types, and performs no checks as to their relationship.
While I can see why you want something like that, I wonder if it is possible to be more precise as to what it should do.
And as a result, potentially make it a bit safer.
It seems like a more specific function can replace the offset() uses in this module.
Essentially, a function that is like offset(), but with T being specifically *const u8.
And this would allow the name and the semantics to be more precise as well.
It would be called something like at_offset(), or, if we want to be more verbose, val_at_offset(). Or something like that.
For the higher level abstractions, "slice" is a well known abstraction.
But I do not know if there is a common terminology for pointers.
If we were working with slices, it could be called as_slice_transmute().
Maybe for pointers it should be called at_offset_transmute()?
transmute() docs for reference.
(Even for transmute() the compiler is trying to provide some safety.)
/// Return a pointer to a type `T` give there is an instance of `T` at the specified offset.
///
/// # Safety
///
/// The caller must ensure that the `ptr` is valid, and `ptr + offset` points to bytes that are
/// properly aligned for `T` and represent a bit pattern that is a valid instance of `T`.
///
/// If any of this requirements is not valid, this function leads to undefined behavior.
#[inline(always)]
const unsafe fn at_offset_transmute<T>(ptr: *const u8, offset: usize) -> *const T {There was a problem hiding this comment.
I like your suggestion – went with a slightly modified variation:
const unsafe fn field_at_offset<T, U>(ptr: *const T, offset: usize) -> *const U;The function is essentially extracting a pointer to a type U that is a field of a type T at the specified offset.
There was a problem hiding this comment.
Now this commit contains changes beside just comments on constants :P
There was a problem hiding this comment.
True – updated the PR name and description to better reflect the changes in it.
I do not have a very strong opinion one way or another. |
febo
force-pushed
the
febo/comments-on-constants
branch
from
7e68d0b to
ecc42e9
Compare
sdk/pinocchio/src/instruction.rs
Outdated
| /// Return a pointer to a type `U` given a source pointer for `T` and the relative offset | ||
| /// to `U` in units of `T`. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller must ensure that the `source` pointer is valid for `T` and that the offset is | ||
| /// within the bounds of the memory region pointed to by `source`. The resulting pointer must | ||
| /// at an aligned memory location that can be safely cast to `*const U`. | ||
| /// | ||
| /// If any of this requirements is not valid, this function leads to undefined behavior. | ||
| #[inline(always)] | ||
| const fn offset<T, U>(ptr: *const T, offset: usize) -> *const U { | ||
| unsafe { (ptr as *const u8).add(offset) as *const U } | ||
| const unsafe fn offset<T, U>(source: *const T, offset: usize) -> *const U { |
There was a problem hiding this comment.
Now this commit contains changes beside just comments on constants :P
| performant | ||
| syscall/S | ||
| syscall/S | ||
| bitmask |
There was a problem hiding this comment.
minor
Seems like this file does not have a new line at the end.
Here is one set of arguments as to why this might matter: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
febo
changed the title
author Fernando Otero <[email protected]> 1752707789 +0100 committer zubayr1 <[email protected]> 1752784793 +0200 Clarify the use of constant values (anza-xyz#200) * Add comments on constants * Improve offset comments * Add bitmask to dictionary * Renamed to field_at_offset Ignore `zero_init` parameter (anza-xyz#203) Ignore zero_init parameter issue 136 idea solution add checked accounts fix instruction_data argument for lamports use Sysvar rent account update mod fix typo issue 136 idea solution fix instruction_data argument for lamports update mod fix typo implement invoke_signed_checked
Problem
There are a few places where "raw" values (magic numbers) are being used, which might not clearly describe their use.
Solution
This PR improves the documentation for these values. Additionally, it updates the name of the function used to calculate a pointer offset to a field of a type (
offset->field_at_offset) to better represent its purpose – this is a function used with "raw" values.This is an alternative to PR #191