fix: avoiding usage of _ if already defined

[henryiii]

Description

Closes #3401 by avoiding using a raw _.

Suggested changelog entry:

* Replace ``_`` with ``const_name`` in internals, avoid defining ``pybind::_`` if ``_`` defined as macro (common gettext usage).

[jagerman]

This is a majorly backwards incompatible change because it breaks documented practices for custom type casters since pretty much forever (see https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html, and the fact that you had to change the custom casters in test code to get it to compile).

[henryiii]

Didn't realize this was so public, but was mostly playing around to see what it would take. The _("") is really non-descriptive (and does, as the issue states, interfere with a very common practice for doing translations). We could provide a backward compatible definition of _ that could would be disabled if a _ macro was defined.

Also, am I correct in my assessment that it's really just for compile time manipulatable strings?

Found some useful docs improvements that I'll probably pull out.

[henryiii]

I might try the "_" only if no macro exists idea.

[henryiii]

This renames _ to const_str, and also defines _, but only if not already defined as a macro. I can move a few of the tests back to using _ so we cover both. We can also move the docs to recommending the new spelling, or just keep it as an internal detail for cases where there is a conflict. Personally, I think it reads better instead of _(), which is usually means it supports translations. Thoughts?

[jagerman]

Also, am I correct in my assessment that it's really just for compile time manipulatable strings?

It's been a while, but yes, as I recall, that is the main purpose -- by being compile-time constants they can avoid getting compiled into the actual final code. I think @wjakob probably chose _ because it was syntactically minimal.

Replacing it for 3.0 definitely seems good to me; in the meantime, perhaps it could go behind an opt-in macro?

[henryiii]

The current system seems pretty safe and backward compatible; both const_str and _ are available, internally only const_str is used, and if _ is defined as a macro, then _ is not available, just const_str. Maybe we could then drop it in 3.0, but it's pretty minimal (just four lines or so), so not that hard to maintain.

Basically it's an opt-out macro, and the macro is exactly the thing it collides with anyway.

[jagerman]

This needs a doc update, too: the _ is still used in the examples in https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html

[henryiii]

Do we want to change the docs over? I think it's more readable, so I'd be in favor. We can leave _ available indefinitely due to its previous prevalence.

[rwgk]

Do we want to change the docs over? I think it's more readable, so I'd be in favor. We can leave _ available indefinitely due to its previous prevalence.

I'm strongly in favor if changing the docs to const_str. I'm really glad to see this change, the _ is so easy to overlook, so difficult to pin-point, and gives no clue to what it's for.

[rwgk]

Randomly crossed my mind:

const_char_array would be a more truthful / expressive name.

py::str is something completely different from py::detail::const_str.

[jagerman]

Randomly crossed my mind:

const_char_array would be a more truthful / expressive name.

Kind of, but the point of this function isn't to be a const char array, it's meant only to be a compile-time static name for a class, and being a const char array just happens to be the current implementation detail. So perhaps "py::type_name" or something along that line would be a better choice?

[henryiii]

I think the overlap between py::detail::const_str and py::str is fine - there can't be a const version of py::str, since it's a Python object, and you really don't see the py::detail part, it's always just const_str in the type caster. It's important to keep it fairly clear and short. _ is a bit too much, but const_char_array would be too long. (And, eventually, it might be able to use a constexpr string, perhaps?)

const_name or static_name or static_str are ideas, though.

I can add a test. I'm wondering if adding a _ define is tripping up Boost.

[rwgk]

const_name or static_name or static_str are ideas, though.

const_name sounds good to me, in line with Jason's idea to emphasize high level vs impl detail.

static has so many meanings already ... thinking, better not.

[henryiii]

So const_str, const_name, and type_name (which I missed from before).

[rwgk]

Const name is my strong favorite (Typing on my phone)

…

On Fri, Dec 17, 2021 at 12:26 Henry Schreiner ***@***.***> wrote: So const_str, const_name, and type_name (which I missed from before). — Reply to this email directly, view it on GitHub <#3423 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAAKL5GKD2GPHU2POWTUROMHVANCNFSM5G5SOKAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[rwgk]

I just took another look. Perfect, thanks a lot for doing this!

[henryiii]

Can I get one more approval? @jagerman, @Skylion007, or someone else?

[jagerman]

Looks good to me.