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

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <bcain@quicinc.com>
> Sent: Sunday, October 8, 2023 7:50 AM
> To: ltaylorsimpson@gmail.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
> 
> 
> 
> > -----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)
> >
> >
> 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/7f20565d403d16337ab6d69ee663121
> a3eef71e6
> 
> 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?

Yes.  It looked like some adding the v_ would sometimes put the line over the 80 character size.
If so, wrap them as described.  If not, no wrapping is needed.


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