Re: [PATCH 1/5] gdb/python: handle saving user registers in a frame unwinder

Andrew Burgess
2021-06-07T20:12:55Z

* Lancelot SIX <lsix@lancelotsix.com> [2021-06-07 19:01:31 +0100]:

> On Mon, Jun 07, 2021 at 01:20:33PM -0400, Simon Marchi wrote:
> > On 2021-06-07 1:07 p.m., Lancelot SIX via Gdb-patches wrote:
> > > Hi,
> > >
> > > I just have a minor stylistic remark in the python code in the test:
> > >
> > >> […]
> > >> +
> > >> + def __call__(self, pending_frame):
> > >> + pc_desc = pending_frame.architecture().registers().find("pc")
> > >> + pc = pending_frame.read_register(pc_desc)
> > >> +
> > >> + sp_desc = pending_frame.architecture().registers().find("sp")
> > >> + sp = pending_frame.read_register(sp_desc)
> > >> +
> > >> + block = gdb.block_for_pc(int(pc))
> > >> + if block == None:
> > >
> > > When looking for None, it is usually prefered to use 'is None' instead
> > > of '== None'. The result is the same unless there is a strange overload
> > > of __eq__.
> > >
> > > This pattern can also be seen in patch 3 and 4 of your series (patch 4
> > > using both '==' and 'is' to check for None).
> >
> > I agree, that's the convention in Python. It is not in our coding
> > standards, but I suggest using flake8 to check the Python code, it
> > reports this (and much more):
>
> Hi,
>
> Actually, this is mentioned in the PEP-8[1][2], which states in the
> “Programming Recommandations” section:
>
> Comparisons to singletons like None should always be done with is or
> is not, never the equality operators.
>
> This leads me to an annex question. Given that I still lack a lot of
> experience with the overall codebase, I tend to pick this kind of small
> stylistic details more easily than design and logic problems. I do not
> always point out those I see when I read the ML, but I can totally
> understand those isolated stylistic comments can be considered as noise.
> If so, please let me know!

I agree with Simon that any constructive feedback is great.

And specifically, thanks for pointing this issue out to me.

I've updated all of the patches locally to use 'is None' now.

Thanks again,
Andrew