> -----Original Message-----
> From: Brian Cain <bc...@quicinc.com>
> Sent: Sunday, October 8, 2023 7:50 AM
> To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org
> Cc: arm...@redhat.com; richard.hender...@linaro.org; phi...@linaro.org;
> peter.mayd...@linaro.org; Matheus Bernardino (QUIC)
> <quic_mathb...@quicinc.com>; stefa...@redhat.com; a...@rev.ng;
> a...@rev.ng; Marco Liebel (QUIC) <quic_mlie...@quicinc.com>
> Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
>
>
>
> > -----Original Message-----
> > From: ltaylorsimp...@gmail.com <ltaylorsimp...@gmail.com>
> > Sent: Friday, October 6, 2023 11:01 AM
> > To: Brian Cain <bc...@quicinc.com>; qemu-devel@nongnu.org
> > Cc: arm...@redhat.com; richard.hender...@linaro.org;
> > phi...@linaro.org; peter.mayd...@linaro.org; Matheus Bernardino (QUIC)
> > <quic_mathb...@quicinc.com>; stefa...@redhat.com; a...@rev.ng;
> > a...@rev.ng; Marco Liebel (QUIC) <quic_mlie...@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 <bc...@quicinc.com>
> > > Sent: Thursday, October 5, 2023 4:22 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: bc...@quicinc.com; arm...@redhat.com;
> > > richard.hender...@linaro.org; phi...@linaro.org;
> > > peter.mayd...@linaro.org; quic_mathb...@quicinc.com;
> > > stefa...@redhat.com; a...@rev.ng; a...@rev.ng;
> > > quic_mlie...@quicinc.com; ltaylorsimp...@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 <bc...@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 <ltaylorsimp...@gmail.com>
> >