RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Brian Cain]

> -----Original Message-----
> From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> Sent: Friday, October 6, 2023 11:01 AM
> To: Brian Cain <bcain@quicinc.com>; qemu-devel@nongnu.org
> Cc: armbru@redhat.com; richard.henderson@linaro.org; philmd@linaro.org;
> peter.maydell@linaro.org; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>; stefanha@redhat.com; ale@rev.ng;
> anjo@rev.ng; Marco Liebel (QUIC) <quic_mliebel@quicinc.com>
> Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> > -----Original Message-----
> > From: Brian Cain <bcain@quicinc.com>
> > Sent: Thursday, October 5, 2023 4:22 PM
> > To: qemu-devel@nongnu.org
> > Cc: bcain@quicinc.com; armbru@redhat.com; richard.henderson@linaro.org;
> > philmd@linaro.org; peter.maydell@linaro.org; quic_mathbern@quicinc.com;
> > stefanha@redhat.com; ale@rev.ng; anjo@rev.ng;
> > quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com
> > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> >
> > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
> > identifiers to avoid shadowing the type name.
> >
> > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
> > rename the function arguments to avoid shadowing the global.
> >
> > Signed-off-by: Brian Cain <bcain@quicinc.com>
> > ---
> >  target/hexagon/genptr.c                 | 56 ++++++++++++-------------
> >  target/hexagon/genptr.h                 | 18 ++++----
> >  target/hexagon/mmvec/system_ext_mmvec.c |  4 +-
> > target/hexagon/mmvec/system_ext_mmvec.h |  2 +-
> >  target/hexagon/op_helper.c              |  4 +-
> >  5 files changed, 42 insertions(+), 42 deletions(-)
> >
> > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> > 217bc7bb5a..11377ac92b 100644
> > --- a/target/hexagon/genptr.c
> > +++ b/target/hexagon/genptr.c
> > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result,
> TCGv
> > src)
> >      tcg_gen_deposit_i64(result, result, src64, N * 8, 8);  }
> >
> > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int
> > mem_index)
> > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int
> > +mem_index)
> 
> I'd recommend moving both the type and the arg name to the new line, also
> indent the new line.
> static inline void gen_load_locked4u(TCGv dest, TCGv v_addr,
>                                                                   int 
> mem_index)
> 
> 
> >
> > -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int
> > mem_index)
> > +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int
> > +mem_index)
> 
> Ditto
> 
> >  static inline void gen_store_conditional4(DisasContext *ctx,
> > -                                          TCGv pred, TCGv vaddr, TCGv src)
> > +                                          TCGv pred, TCGv v_addr, TCGv
> > + src)
> 
> Ditto
> 
> >      zero = tcg_constant_tl(0);
> > @@ -374,13 +374,13 @@ static inline void
> > gen_store_conditional4(DisasContext *ctx,  }
> >
> >  static inline void gen_store_conditional8(DisasContext *ctx,
> > -                                          TCGv pred, TCGv vaddr, TCGv_i64 
> > src)
> > +                                          TCGv pred, TCGv v_addr,
> > + TCGv_i64 src)
> 
> Indent
> 
> > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int
> > slot)
> > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int
> > +slot)
> 
> Ditto
> 
> > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int
> > slot);
> > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int
> > +slot);
> 
> Ditto

I could be mistaken but AFAICT none of these lines are wrapped in the way 
they're quoted above  in my patch (nor the baseline).  I don't think any of 
these lines exceed 80 columns, so they shouldn't need wrapping, either.

I double checked how it's displayed at the archive 
https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to make 
sure that it wasn't a misconfiguration of my mailer.  For another perspective - 
refer to the commit used to create this patch: 
https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121a3eef71e6

Is your review comment that "these lines should be wrapped and when you do, 
make sure you do it like this"?  Or "if you are going to wrap them, wrap them 
like this"?  Or something else?

> Otherwise,
> Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
>