Clean up dict_del_by_value

[jdemeyer]

Depends on #23413

Component: cython

Author: Jeroen Demeyer

Branch/Commit: u/jdemeyer/ticket/23414 @ cc7de2c

Issue created by migration from https://trac.sagemath.org/ticket/23414

[jdemeyer]

Branch: u/jdemeyer/ticket/23414

[nbruin]

comment:2

I intentionally kept the definitions of PyDictEntry etc. out of the pxd: I didn't feel that it was the task of that pxd to provide interfaces to CPython API, in particular because they are Py2/Py3 dependent. Do you have a good reason to start exposing these in the pxd? Also, differentiating between the different fields depending on implementation by just comments seems a little fragile to me. It punts all errors to the c compile, whereas the cythonization can already fail here.

---

New commits:

cf224b7	Implement wrapperdescr_call without checking
9a4ef8c	Wording
ed0d88c	Move various things to src/sage/cpython
cc7de2c	Clean up dict_del_by_value

[nbruin]

Commit: cc7de2c

[jdemeyer]

comment:3

Replying to @nbruin:

Do you have a good reason to start exposing these in the pxd?

The main reason is to eventually move those to Cython itself. Usually, Cython upstream welcomes additional declarations regarding the CPython API.

[nbruin]

comment:5

Replying to @jdemeyer:

The main reason is to eventually move those to Cython itself. Usually, Cython upstream welcomes additional declarations regarding the CPython API.

OK. The part of the API that gets exposed is indeed part of what Python.h publishes. However, it is a bit that is dependent on Py2/Py3. Currently, the header file basically publishes the union, leaving it to the C compiler afterwards to raise an error in case of a python version mismatch. So code that includes this header file needs to guard on Py2/Py3.

On the other hand, the file backports PyDict_GetItemWithError, which works towards writing code that is Py2/Py3 agnostic.

Furthermore, the main purpose of this module is to provide a function that is badly bound to unpublished implementation details of dictionaries. As far as I know, the routine provided here is only a good idea for an improved implementation of WeakValueDict. So its pxd would ideally only include del_dictitem_by_exact_value, so that people only create dependencies on this module if they really need to.

I'm a little hesitant to name this a good API choice. Since this ticket is only about refactoring, I think we only want to merge it if it actually improves the API and the reasons above make me doubt that.

I don't think the change proposed is disastrous either, so if there's another reviewer who thinks the proposed change really does provide an improvement, I won't argue.

The easiest way to accomplish integration into cython is probably just to generate a pull request to include these definitions into site-packages/Cython/Includes/cpython/dict.pxd.

It'll be relatively complicated, because Cython tries to generate C-code that can be compiled against Py2 as well as Py3 and they are fundamentally incompatible for this.

[jdemeyer]

comment:6

When talking about moving to Cython, I meant just the declarations of the structs, not everything in this Sage module.

[jdemeyer]

comment:7

Replying to @nbruin:

The easiest way to accomplish integration into cython is probably just to generate a pull request to include these definitions into site-packages/Cython/Includes/cpython/dict.pxd.

I usually prefer to put the code in Sage first, as some kind of testcase. When it's been shown that the Sage code works, I'll make a pull request. Otherwise, you might end up in a situation where you first make a pull request and then realize that it doesn't quite work.

[nbruin]

comment:8

Replying to @jdemeyer:

When talking about moving to Cython, I meant just the declarations of the structs, not everything in this Sage module.

Yes, I understand. I think moving those to cython would be fine.

The problem with the structs is that these are incompatible between Py2/Py3 and that code for one will not compile on the other. And I'm not sure that taking the union of the two on the cython level is the best way to handle (hide) this incompatibility. I also don't think sage is a great testing ground for Py2/Py3 compatibility just yet, because it doesn't really get compiled on Py3 yet.

My main concern is that this module, because it implements relatively dangerous stuff, is not an appropriate testing ground either.

[jdemeyer]

comment:9

Replying to @nbruin:

My main concern is that this module, because it implements relatively dangerous stuff, is not an appropriate testing ground either.

Well, it is a testing ground to ensure that stuff compiles, which is really the only thing which could go wrong with the struct declarations.

[jdemeyer]

comment:10

Replying to @nbruin:

The problem with the structs is that these are incompatible between Py2/Py3 and that code for one will not compile on the other. And I'm not sure that taking the union of the two on the cython level is the best way to handle (hide) this incompatibility.

It's simple, it works. Why not?

[nbruin]

comment:11

Replying to @jdemeyer:

It's simple, it works. Why not?

It just looks wrong to me because the struct definition is not a subset of the fields available (not on Py2 and not on Py3), and there's a struct definition that is not available on Py3.

I was hoping to help and review this ticket, but I don't feel this is within my expertise. I'd be comfortable if the different structs were guarded by a PY_HEX_VERSION or similar, because then it is clearly declaring the right thing.

[nbruin]

comment:12

Replying to @jdemeyer:

Replying to @nbruin:

My main concern is that this module, because it implements relatively dangerous stuff, is not an appropriate testing ground either.

Well, it is a testing ground to ensure that stuff compiles, which is really the only thing which could go wrong with the struct declarations.

That's not the issue: You're adding functionality to the pxd, giving a reason to include this pxd for reasons other than including del_dictitem_by_exact_value.

[fchapoton]

comment:13

does not apply