Replace error printing code gated by NDEBUG with a new flag: PYBIND11_DETAILED_ERROR_MESSAGES

[voznesenskym]

Description

Does what it says in the title. We have some internal use cases of pybind where we would love to get richer logging from within pybind, but are not running a debug build.

Suggested changelog entry:

* Error printing code now uses ``PYBIND11_DETAILED_ERROR_MESSAGES`` instead of requiring ``NDEBUG``.

[voznesenskym]

Apologies upfront if this is not desired. I am not 100% sure how to test it, I did run all unit tests and nothing exploded.

[voznesenskym]

Only those with write access to this repository can merge pull requests.

@Skylion007 Thanks for the stamp, I do not have write access :)

[Skylion007]

Any objections @henryiii or @rwgk ? If not, let's merge it.

[rwgk]

The general idea is great, but the implementation needs more attention.

The prefix for macros is universally PYBIND11_ (11 is currently missing).

LOG isn't very fitting, let's think carefully about naming. I think ERROR definitely needs to be in the name, maybe PYBIND11_DETAILED_ERROR_MESSAGES?

"compile in" appears in these files:

cast.h
attr.h
pybind11.h
detail/type_caster_base.h

After a quick pass looking at those files, I'm thinking it is best to replace NDEBUG with the new macro for all "compile in" matches. With that comes the idea to handle the definition of the new macro in detail/common.h.

The old phrase "compile in debug mode for ..." is best updated, e.g. "compile with PYBIND11_DETAILED_ERROR_MESSAGES defined for ..." will be more to the point and helpful.

The current #if logic definitely needs work:

#if !defined(PYBIND11_DETAILED_ERROR_MESSAGES) && !defined(NDEBUG)
#    define PYBIND11_DETAILED_ERROR_MESSAGES
#endif

Optional, or follow-on PR: define the macro for at least one non-debug CI job (very easy) and verify that some error is complete with details (needs some thought, unless someone already has a good idea how to implement that).

[voznesenskym]

Great suggestions, one and all. I’ll get to this tonight. Cheers.

…

On Thu, Apr 28, 2022 at 11:31 PM Ralf W. Grosse-Kunstleve < ***@***.***> wrote: ***@***.**** requested changes on this pull request. The general idea is great, but the implementation needs more attention. The prefix for macros is universally PYBIND11_ (11 is currently missing). LOG isn't very fitting, let's think carefully about naming. I think ERROR definitely needs to be in the name, maybe PYBIND11_DETAILED_ERROR_MESSAGES ? "compile in" appears in these files: cast.h attr.h pybind11.h detail/type_caster_base.h After a quick pass looking at those files, I'm thinking it is best to replace NDEBUG with the new macro for all "compile in" matches. With that comes the idea to handle the definition of the new macro in detail/common.h. The old phrase "compile in debug mode for ..." is best updated, e.g. "compile with PYBIND11_DETAILED_ERROR_MESSAGES defined for ..." will be more to the point and helpful. The current #if logic definitely needs work: #if !defined(PYBIND11_DETAILED_ERROR_MESSAGES) && !defined(NDEBUG) # define PYBIND11_DETAILED_ERROR_MESSAGES #endif Optional, or follow-on PR: define the macro for at least one non-debug CI job (very easy) and verify that some error is complete with details (needs some thought, unless someone already has a good idea how to implement that). — Reply to this email directly, view it on GitHub <#3913 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABEI6NCNF5EEPIPYA35ZB2LVHN65XANCNFSM5UT72ZWA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[voznesenskym]

@rwgk thanks for the great feedback. Applied.

w/r/t

Optional, or follow-on PR: define the macro for at least one non-debug CI job (very easy) and verify that some error is complete with details (needs some thought, unless someone already has a good idea how to implement that).

There are tests in tests/test_methods_and_attributes.py and tests/test_pytypes.py that test that the correct mode prints the correct value. It's not perfect, as it relies on debug_enabled being set, which is set from reading the right #define, but can of course be set by anyone, anywhere.

[rwgk]

Looks great to me!

@Skylion007, do you want to take another look?