[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
From: |
ltaylorsimpson |
Subject: |
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals |
Date: |
Mon, 9 Oct 2023 12:59:53 -0600 |
> -----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>
> >
- [PATCH v2 0/3] hexagon: GETPC() and shadowing fixes, Brian Cain, 2023/10/05
- [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Brian Cain, 2023/10/05
- Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Philippe Mathieu-Daudé, 2023/10/09
- Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Philippe Mathieu-Daudé, 2023/10/09
- RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Brian Cain, 2023/10/09
- Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Philippe Mathieu-Daudé, 2023/10/10
- Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Markus Armbruster, 2023/10/10
- Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Philippe Mathieu-Daudé, 2023/10/10
- Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Thomas Huth, 2023/10/10
- RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Brian Cain, 2023/10/17
- Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals, Markus Armbruster, 2023/10/10
[PATCH v2 2/3] target/hexagon: fix some occurrences of -Wshadow=local, Brian Cain, 2023/10/05