bpo-42093: Cleanup _PyDict_GetItemHint()

[vstinner]

• No longer save/restore the current exception. It is no longer used
  with an exception raised.
• No longer clear the current exception on error: it's now up to the
  caller.

https://bugs.python.org/issue42093

[vstinner]

@pablogsal @methane: Would you mind to review this PR? I'm not sure why PyErr_Fetch/PyErr_Restore was used in the first place, but it is no longer needed.

This PR makes _PyDict_GetItemHint() simpler.

[pablogsal]

Can you run the test suite without --pydebug? Remember that the opcode cache runs only in non debug mode. We should do something about that because is quite annoying to test.

[gvanrossum]

Remember that the opcode cache runs only in non debug mode. We should do something about that because is quite annoying to test.

Why was this done in the first place? I hit this several times...

[vstinner]

@gvanrossum: "Why was this done in the first place? I hit this several times..."

It's a workaround to be able to detect memory leaks on "Refleaks" buildbots: see https://bugs.python.org/issue37146

/* per opcode cache */ #ifdef Py_DEBUG // --with-pydebug is used to find memory leak. opcache makes it harder. // So we disable opcache when Py_DEBUG is defined. // See [bpo-37146](https://bugs.python.org/issue37146) #define OPCACHE_MIN_RUNS 0 /* disable opcache */ (...) 

[vstinner]

I built Python in release mode with assertions:

make clean ./configure sed -i -e 's!-DNDEBUG!!g' Makefile make 

The test suite pass succesfully:

$ ./python -m test -j0 -r (...) 406 tests OK. 

[gvanrossum]

Does PEP 7 say anything about } else { vs. starting the else on a new line?

[methane]

Does PEP 7 say anything about } else { vs. starting the else on a new line?

Yes.

https://www.python.org/dev/peps/pep-0007/#code-lay-out

All new C code requires braces. Braces should be formatted as shown:

if (mro != NULL) { ... } else { ... }

[vstinner]

Does PEP 7 say anything about } else { vs. starting the else on a new line?

Honestly, I don't care much about which style is the best or not. I only care about using the same style on all C files.

Right now, we are far from consistency, since tons of C code is old and the PEP 7 only sets a rule on braces since "recently" (5 or 10 years ago? :-D).

There is a draft PEP about reformatting the whole C code at once, but I'm not sure that if it's going to succeed. In the meanwhile, sometimes I take the opportunity to reformat the change "around" my change, but I try to restrict myself to avoid changes which are only about reformating ;-)

My greatest pleasure is to use C99 (thanks to updated PEP 7, since Python 3.6) to move variable declaration to where the variables are initialized. IMO it really helps readability, since the variable have a more narrow scope. For example, it's simpler to check for reference leaks with shorter scope. I also love C99 way to initialize structure: {.member = value} syntax, it also helps a lot for readability! Example of Modules/_abc.c:

static struct PyModuleDef _abcmodule = { PyModuleDef_HEAD_INIT, .m_name = "_abc", .m_doc = _abc__doc__, .m_size = sizeof(_abcmodule_state), .m_methods = _abcmodule_methods, .m_slots = _abcmodule_slots, .m_traverse = _abcmodule_traverse, .m_clear = _abcmodule_clear, .m_free = _abcmodule_free, }; 

[vstinner]

@methane and @gvanrossum: Thanks for the review, I merged my PR.

We have 2 dict functions doing "crazy things" with tstate and exceptions:

• PyDict_GetItem(): 59d3dce
• _PyDict_GetItemHint(): d5fc998

Oh now I understand: _PyDict_GetItemHint() was copied from PyDict_GetItem(), before I modified PyDict_GetItem().

PyDict_GetItem() still saves/restores the current exception, but I don't think that we will ever be able to change the default behavior: PyDict_GetItemWithError() must be used instead.
https://docs.python.org/dev/c-api/dict.html#c.PyDict_GetItemWithError

@serhiy-storchaka replaces many PyDict_GetItem() calls with PyDict_GetItemWithError(). For example in bpo-35459: a24107b.

I also dislike that all dict "Get" C functions return a borrow reference.

Aaaah, legacy ;-)

[gvanrossum]

Does PEP 7 say anything about } else { vs. starting the else on a new line?

Honestly, I don't care much about which style is the best or not. I only care about using the same style on all C files.

I wasn't asking for an editorial :-) I just didn't recall whether PEP 7 said anything about this situation. I had looked but didn't find it, my bad. Glad Inada linked me to it.

Right now, we are far from consistency, since tons of C code is old and the PEP 7 only sets a rule on braces since "recently" (5 or 10 years ago? :-D).

There is a draft PEP about reformatting the whole C code at once, but I'm not sure that if it's going to succeed. In the meanwhile, sometimes I take the opportunity to reformat the change "around" my change, but I try to restrict myself to avoid changes which are only about reformating ;-)

I'd be happy with this, since it would mean it would be harder to find lines of code that I wrote. :-)

My greatest pleasure is to use C99 (thanks to updated PEP 7, since Python 3.6) to move variable declaration to where the variables are initialized. IMO it really helps readability, since the variable have a more narrow scope. For example, it's simpler to check for reference leaks with shorter scope.

Yes, I love this too!

I also love C99 way to initialize structure: {.member = value} syntax, it also helps a lot for readability! Example of Modules/_abc.c:

static struct PyModuleDef _abcmodule = { PyModuleDef_HEAD_INIT, .m_name = "_abc", .m_doc = _abc__doc__, .m_size = sizeof(_abcmodule_state), .m_methods = _abcmodule_methods, .m_slots = _abcmodule_slots, .m_traverse = _abcmodule_traverse, .m_clear = _abcmodule_clear, .m_free = _abcmodule_free, }; 

+1000 on this. Even if we don't reformat all the code maybe we should reformat all the type definitions using this, it helps a lot, and we could drop the silly comments everywhere.

[vstinner]

+1000 on this. Even if we don't reformat all the code maybe we should reformat all the type definitions using this, it helps a lot, and we could drop the silly comments everywhere.

There is an on-going work to convert static types to heap types. We take it as an opportunity to use C99 initializer syntax ;-) So it's just purely "coding style" changes. https://bugs.python.org/issue40077