qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset


From: Zhao Liu
Subject: Re: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
Date: Thu, 23 Jan 2025 01:00:54 +0800

> > > -    pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> 
> > > std::ops::ControlFlow<u64, u64> {
> > > +    fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, 
> > > u32> {
> > >           use RegisterOffset::*;
> > 
> > Can we move this "use" to the start of the file?
> 
> I don't think it's a good idea to make the register names visible
> globally...  "use Enum::*" before a match statement is relatively common.
> For example: https://doc.rust-lang.org/src/std/io/error.rs.html#436

Thanks!

> > > +    pub fn read(&mut self, offset: hwaddr, _size: u32) -> 
> > > ControlFlow<u64, u64> {
> > 
> > Maybe pub(crate)? But both are fine for me :-)
> 
> The struct is not public outside the crate, so it doesn't make a difference,
> does it?

You're right.

> > Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> 
> Thanks, I'll post a quick v2 anyway once you've finished reviewing.
> 

The remaining critical ones, I'll go through them all tomorrow.

Regerds,
Zhao




reply via email to

[Prev in Thread] Current Thread [Next in Thread]