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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2022-08-17 18:04, Maciej W. Rozycki 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.

> 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

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.

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.

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.

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?

Thanks,

Simon