Skip to content

changelog for #2705 #2728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

changelog for #2705 #2728

wants to merge 1 commit into from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Dec 16, 2020

#2705 could be seen as adding a feature: const/non-const propagation that wasn't supported before. I think a changelog is useful, but I'd be happy to drop this, up to you.

@YannickJadoul
Copy link
Collaborator

I'm not a great fan of advertising const-correctness in the casters until we've checked/fixed this in general. (Though I'm very much up for actually having this in pybind11!)

But it's also not wrong, so ... eh. Tending to "I wouldn't", but not strongly.

If this is considered a new feature, strictly speaking it should be 2.7.0 instead of 2.6.2, though, according to Semantic Versioning (as I tagged #2705, a handful of minutes ago).

@henryiii
Copy link
Collaborator

If we don't have proper tests for it and/or it's hard to use so far, though I'm normally a fan of a detailed changelog, I could go either way in this case. It's a pretty specific statement, not like it's advertising complete const correctness.

@YannickJadoul
Copy link
Collaborator

If we don't have proper tests for it and/or it's hard to use so far, though I'm normally a fan of a detailed changelog, I could go either way in this case. It's a pretty specific statement, not like it's advertising complete const correctness.

That's also true, yes.
If I had written it, I would be slightly more careful, phrasing it similar to the original "make std::reference_wrapper act more like an ordinary C++ reference" (or, as I said in #2705, if it's considered a bugfix, I wouldn't even bother trying to explain the change). But if @rwgk prefers this, sure, why not.

We should only be a bit careful with SemVer on whether this is a bugfix or feature, cfr. which future version of pybind11 is this part of (2.6.2 or 2.7.0).

@rwgk
Copy link
Collaborator Author

rwgk commented Dec 16, 2020

If we don't have proper tests for it

We do! I haven't tried it out myself, but it is my understanding that some of @laramiel's test additions fail without his change, and work with his change.

In any case, I agree that this isn't very important to have in the changelog. I created it assuming Henry would ask for it. I'll close this PR.

@rwgk rwgk closed this Dec 16, 2020
@YannickJadoul
Copy link
Collaborator

Up to @henryiii or @bstaletic, if they still want a changelog entry?

I'm not strongly against, but I have my reservations (as phrased above :-) ).

We do! I haven't tried it out myself, but it is my understanding that some of @laramiel's test additions fail without his change, and work with his change.

That's great to know, though! What I meant is mostly that I don't know how these kinds of const-preserving casters interfere with other built-in casters (like tuple etc), though; so that's why I'm a bit cautious about proclaiming "const-correctness".
If we start a thorough review of the casters and everything does work (not just tests, though; I'd like to see this fit in to the global architecture), that would be really nice, though! If it's easy enough, I could even envision some future (pybind11 2.8?) built-in functionality that will preserve const?

@rwgk rwgk deleted the pr2705_changelog branch December 17, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants