Apply clang-tidy fixes to subinterpreter support code #5929
Conversation
Signed-off-by: cyy <cyyever@outlook.com>
Signed-off-by: cyy <cyyever@outlook.com>
PyObject_HasAttrString may return -1 to signal an error and set a Python exception. The previous logic only checked for "!= 0", which meant that the error path was treated the same as "attribute exists", causing two problems: misreporting the presence of __notes__ and leaving a spurious exception pending. The earlier PR tightened the condition to "== 1" so that only a successful lookup marks the error string as [WITH __notes__], but it still left the -1 case unhandled. In the context of error_fetch_and_normalize, we are already dealing with an active exception and only want to best-effort detect whether normalization attached any __notes__. If the attribute probe itself fails, we do not want that secondary failure to affect later C-API calls or the error we ultimately report. This change stores the PyObject_HasAttrString return value, treats "== 1" as "has __notes__", and explicitly calls PyErr_Clear() when it returns -1. That way, we avoid leaking a secondary error while still preserving the original exception information and hinting [WITH __notes__] only when we can determine it reliably.
|
@cyyever WDYT about changing our clang-tidy job to run with Our CI resources are very limited, therefore just one job is probably most practical. |
|
@rwgk For limited resources, running tidy and fixing errors periodically is enough. There is no need to modify current jobs. |
Hm, according to a quick analysis done with Cursor, we'll get around 350 lines more covered if we use |
rwgk
changed the title
|
Thanks for doing this cleanup and for running clang-tidy over the subinterpreter code paths - having those headers/tests tidied is definitely appreciated. That said, we generally try to avoid one-off style/tidy cleanups that aren't backed by CI, because without an automated check it's very easy for the code to drift back out of compliance. In this particular area we're also constrained by the fact that our current clang-tidy job is tied to Python 3.11, while the subinterpreter APIs you touched only exist in 3.12+, so we don't yet have a good way to enforce these changes automatically. I'm going to merge this PR because the changes themselves look fine and it's useful groundwork, but for future PRs we'd prefer to reserve review/CI bandwidth for changes that either fix user-visible issues, add test coverage, or extend our automation (for example by updating CI/tidy configs). In practice that means tidy-style cleanups are most helpful when they come together with the CI changes needed to keep them from backsliding. |
|
I see |
|
@rwgk |
That's exactly what we want here. The code changed is handling errors. We don't want to handle any non-critical secondary errors during the handling of the primary error. |
2 participants
Description
This PR applies our current clang-tidy checks to the subinterpreter support code (including the embedding tests), using a local environment with CPython >= 3.12 where
PYBIND11_HAS_SUBINTERPRETER_SUPPORTis enabled.At the moment, CI does not run clang-tidy in a configuration that exposes these subinterpreter APIs (our clang-tidy job is still tied to Python 3.11), so this cleanup is not protected against backsliding by automated checks. It's still useful to get the code into a cleaner state now, but the long-term goal should be to extend CI so that these tidy rules are enforced automatically for the subinterpreter paths as well.
The tiny change under PR #5928 was merged into this PR, so that both changes are tested in one CI run (we only have very limited CI resources).