From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id C88273858D3C for ; Mon, 17 Oct 2022 15:03:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C88273858D3C Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 29HF2oib031876 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 17 Oct 2022 11:02:54 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 29HF2oib031876 Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C33891E0D5; Mon, 17 Oct 2022 11:02:49 -0400 (EDT) Message-ID: <0cf90df8-6943-212a-b4c5-bd5301b44369@polymtl.ca> Date: Mon, 17 Oct 2022 11:02:49 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH v6 5/8] GDB/Python: Use None for `var_zuinteger_unlimited' value set to `unlimited' Content-Language: en-US To: "Maciej W. Rozycki" , gdb-patches@sourceware.org Cc: Andrew Burgess , Tom Tromey , Simon Sobisch References: From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 17 Oct 2022 15:02:50 +0000 X-Spam-Status: No, score=-3032.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Oct 2022 15:03:23 -0000 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