| Message ID | CACVxJT-Hj6jdE0vwNrfGpKs73+ScTyxxxL8w_VXfoLAx79mr8w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block, rust: simplify validate_block_size() function | expand |
On Sat, Aug 31, 2024 at 8:52 PM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > Also delete few comments of "increment i by 1" variety. They are not comments, those lines are part of the documentation. Cheers, Miguel
On Sat, Aug 31, 2024 at 09:02:15PM +0200, Miguel Ojeda wrote: > On Sat, Aug 31, 2024 at 8:52 PM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > > Also delete few comments of "increment i by 1" variety. > > They are not comments, those lines are part of the documentation. Why do they duplicate the code 1:1? Ignore the patch BTW, it was mangled by opening gmail Drafts :-(
On Sat, Aug 31, 2024 at 10:13 PM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > Why do they duplicate the code 1:1? I am not sure I understand your question. Documentation establishes what a function is expected to do. Code implements the function. Callers should not need to reverse engineer the design/code to take advantage of a function. Yes, in some cases, it may be trivial to infer what the function is supposed to do by reading its implementation, and thus documentation may seem redundant. But even in simple cases, one still needs to assume the implementation has no bugs in order to do that. And in non-trivial cases, the documentation is even more valuable. It is true that in many places in the kernel there is not much code documentation. However, for Rust code, we are trying to improve on that. Cheers, Miguel
On Sat, Aug 31, 2024 at 10:13 PM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > Ignore the patch BTW, it was mangled by opening gmail Drafts :-( No worries, and thanks for the patch by the way! I leave it up to Andreas whether he wants the range check style or not (I think we should decide on a coding guideline for those and then be consistent). Cheers, Miguel
On Sat, Aug 31, 2024 at 11:42:04PM +0200, Miguel Ojeda wrote: > On Sat, Aug 31, 2024 at 10:13 PM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > > Ignore the patch BTW, it was mangled by opening gmail Drafts :-( > > No worries, and thanks for the patch by the way! > > I leave it up to Andreas whether he wants the range check style or not > (I think we should decide on a coding guideline for those and then be > consistent). I think that consistence is good and imagine that clippy already has an opinion on range check style. Regards Geert Stappers
--- a/rust/kernel/block/mq/gen_disk.rs +++ b/rust/kernel/block/mq/gen_disk.rs @@ -43,21 +43,16 @@ pub fn rotational(mut self, rotational: bool) -> Self { self } - /// Validate block size by verifying that it is between 512 and `PAGE_SIZE`, - /// and that it is a power of two. fn validate_block_size(size: u32) -> Result<()> { - if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() { - Err(error::code::EINVAL) - } else { + if 512 <= size && size <= bindings::PAGE_SIZE as u32 && size.is_power_of_two() { Ok(()) + } else { + Err(error::code::EINVAL) } } /// Set the logical block size of the device to be built. /// - /// This method will check that block size is a power of two and between 512 - /// and 4096. If not, an error is returned and the block size is not set.
Using range and contains() method is just fancy shmancy way of writing two comparisons which IMO is less readable. Using range doesn't prevent any bugs because "=" in range can forgotten just as easily in "<=" operator. Also delete few comments of "increment i by 1" variety. Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> --- rust/kernel/block/mq/gen_disk.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) - /// /// This is the smallest unit the storage device can address. It is /// typically 4096 bytes. pub fn logical_block_size(mut self, block_size: u32) -> Result<Self> { @@ -68,9 +63,6 @@ pub fn logical_block_size(mut self, block_size: u32) -> Result<Self> { /// Set the physical block size of the device to be built. /// - /// This method will check that block size is a power of two and between 512 - /// and 4096. If not, an error is returned and the block size is not set. - /// /// This is the smallest unit a physical storage device can write /// atomically. It is usually the same as the logical block size but may be /// bigger. One example is SATA drives with 4096 byte physical block size