On Mon, Mar 06, 2017 at 09:44:49AM +0000, Peter Maydell wrote:
> On 5 March 2017 at 21:48, Krzysztof Kozlowski <k...@kernel.org> wrote:
> > In few places the function arguments and local variables are not
> > modifying data passed through pointers so this can be made const for
> > code safeness. Also the static array exynos4210_uart_regs is not being
> > modified.
> >
> > Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>
> > ---
> > hw/char/exynos4210_uart.c | 10 +++++-----
> > hw/intc/exynos4210_combiner.c | 2 +-
> > hw/intc/exynos4210_gic.c | 8 ++++----
> > hw/misc/exynos4210_pmu.c | 2 +-
> > 4 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> > index b75f28d473bf..83e1be253255 100644
> > --- a/hw/char/exynos4210_uart.c
> > +++ b/hw/char/exynos4210_uart.c
> > @@ -102,7 +102,7 @@ typedef struct Exynos4210UartReg {
> > uint32_t reset_value;
> > } Exynos4210UartReg;
> >
> > -static Exynos4210UartReg exynos4210_uart_regs[] = {
> > +static const Exynos4210UartReg exynos4210_uart_regs[] = {
> > {"ULCON", ULCON, 0x00000000},
> > {"UCON", UCON, 0x00003000},
> > {"UFCON", UFCON, 0x00000000},
> > @@ -220,7 +220,7 @@ static uint8_t fifo_retrieve(Exynos4210UartFIFO *q)
> > return ret;
> > }
>
> Constifying this sort of thing is OK...
>
> > -static int fifo_elements_number(Exynos4210UartFIFO *q)
> > +static int fifo_elements_number(const Exynos4210UartFIFO *q)
> > {
> > if (q->sp < q->rp) {
> > return q->size - q->rp + q->sp;
> > @@ -229,7 +229,7 @@ static int fifo_elements_number(Exynos4210UartFIFO *q)
> > return q->sp - q->rp;
> > }
> >
> > -static int fifo_empty_elements_number(Exynos4210UartFIFO *q)
> > +static int fifo_empty_elements_number(const Exynos4210UartFIFO *q)
> > {
> > return q->size - fifo_elements_number(q);
> > }
> > @@ -245,7 +245,7 @@ static void fifo_reset(Exynos4210UartFIFO *q)
> > q->rp = 0;
> > }
> >
> > -static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(Exynos4210UartState
> > *s)
> > +static uint32_t exynos4210_uart_Tx_FIFO_trigger_level(const
> > Exynos4210UartState *s)
> > {
> > uint32_t level = 0;
> > uint32_t reg;
>
> ...but I disagree here somewhat...
This is a static function not modifying the state. Const in argument is
a clear indication of that for the readers.
>
> > @@ -488,7 +488,7 @@ static const MemoryRegionOps exynos4210_uart_ops = {
> >
> > static int exynos4210_uart_can_receive(void *opaque)
> > {
> > - Exynos4210UartState *s = (Exynos4210UartState *)opaque;
> > + const Exynos4210UartState *s = (Exynos4210UartState *)opaque;
>
> ...and definitely here.
>
> These are effectively method implementations for QEMU objects,
> and defining the "this" pointer as const in some methods
> because it happens not to be modifying things just makes
> the code look out of line with every other method implementation
I get it, better to keep consistent style.
> in the code base (and means somebody will have to drop the 'const'
> again at some point if the method needs to be updated to
> modify state in the future).
I think the code should be const-correct for current implementation
(following the HACKING guide). If we would have to predict future
changes, then almost no const would be possible to add...
Best regards,
Krzysztof
