Re: [PATCH v6 5/8] GDB/Python: Use None for `var_zuinteger_unlimited' value set to `unlimited'

["Maciej W. Rozycki" <macro@embecosm.com>]

On Mon, 17 Oct 2022, Simon Marchi wrote:

> > Consistently with parameters of the PARAM_UINTEGER and PARAM_INTEGER 
> > types, return the special value of `None' for a PARAM_ZUINTEGER_UNLIMITED 
> 
> I had trouble parsing this sentence, I think adding a comma between
> "types" and "return" would help.

 Fixed in v7.

> > parameter set to `unlimited', fixing an inconsistency introduced with 
> > commit 0489430a0e1a ("Handle var_zuinteger and var_zuinteger_unlimited 
> > from Python"); cf. PR python/20084.  Adjust the testsuite accordingly.
> > 
> > This makes all the three parameter types consistent with each other as 
> > far as the use of `None' is concerned, and similar to the Guile/Scheme 
> > interface where the `#:unlimited' keyword is likewise used.  We have a 
> > precedent already documented for a similar API correction:
> > 
> >  -- Function: gdb.breakpoints ()
> >      Return a sequence holding all of GDB's breakpoints.  *Note
> >      Breakpoints In Python::, for more information.  In GDB version 7.11
> >      and earlier, this function returned 'None' if there were no
> >      breakpoints.  This peculiarity was subsequently fixed, and now
> >      'gdb.breakpoints' returns an empty sequence in this case.
> > 
> > made in the past.
> > 
> > And then we have documentation not matching the interface as far as the 
> > value of `None' already returned for parameters of the PARAM_UINTEGER 
> > and PARAM_INTEGER types is concerned, and the case of an incorrect 
> > assertion for PARAM_UINTEGER and PARAM_ZUINTEGER_UNLIMITED parameters in 
> > the sibling Guile/Scheme module making such parameters unusable that has 
> > never been reported, both indicating that these features may indeed not 
> > be heavily used, and therefore that the risk from such an API change is 
> > likely lower than the long-term burden of the inconsistency.
> 
> To rephrase, just to make sure I understand right, this is the
> inconsistency you'd like to fix:
> 
> (gdb) set an-integer unlimited 
> (gdb) set an-uinteger unlimited 
> (gdb) set a-zuinteger-unlimited unlimited 
> (gdb) python print(gdb.parameter('an-integer'))
> None
> (gdb) python print(gdb.parameter('an-uinteger'))
> None
> (gdb) python print(gdb.parameter('a-zuinteger-unlimited'))
> -1

 Correct.  Please note that the -1 value for PARAM_ZUINTEGER_UNLIMITED was 
also only added when `None' had been already used for PARAM_UINTEGER and 
PARAM_ZINTEGER types, so it was wrong right from the beginning.

> I'm hesitant about this kind of of breaking changes.  I don't agree with
> your reasoning leading you to claim that these features are not heavily
> used.  We had and have all kinds of inconsistencies in our MI and Python
> API, where the actual API doesn't match the doc, and people usually just
> silently work around them.  Also, the fact that Guile was broken doesn't
> mean people don't use the equivalent in Python.

 Fair enough, especially on the last point.

 My overall impression of the Python technical culture has sadly been that 
things largely happen on an ad-hoc basis there, with little structure or 
consistency.  I guess this is an unfortunate side effect resulting from 
the low entry barrier, not by itself bad, Python presents to newcomers to 
computer programming.

> The closest thing to empirical data we can have if searching to
> occurences on Github.  I just did this search, and there are no hits:
> 
>   https://github.com/search?q=PARAM_ZUINTEGER_UNLIMITED+extension%3Apy&type=Code&ref=advsearch&l=&l=
> 
> That is, searching for PARAM_ZUINTEGER_UNLIMITED in all the .py files on
> Github.  As opposed to PARAM_ZINTEGER, for instance:
> 
>   https://github.com/search?q=PARAM_ZINTEGER+extension%3Apy&type=Code&ref=advsearch&l=&l=
> 
> Of course, that's not all the code in the world, but that gives an idea.

 Thank you for doing this research.  I wasn't aware of this Github's 
feature (I don't usually use Github).

> And I do agree that fixing the API once will reduce the long-term costs
> for everybody (us and users bumping in the inconsistency and losing
> time).  So, I am fine with fixing PARAM_ZUINTEGER_UNLIMITED in this
> case.

 Good.  Given that you are in favour to making this change and Andrew was 
against, do you think we need another opinion?  Have we reached consensus?

> However, I am unable to apply your patch locally in order to properly
> review it.  Can you send an updated version of the series, after perhaps
> pushing the already approved patches that make sense on their own?

 I'm not sure what happened here, I had no conflicts in the rebase (but 
then maybe I rebased already after I posted v6 and forgot about it).  I 
have now posted the remaining parts of the series rebased against current 
master.

 Thank you for your review.

  Maciej