Skip to content

fix: Revert pfect args make iterator - #4234

Merged
henryiii merged 3 commits into
pybind:masterfrom
Skylion007:revert-pfect-args-make-iterator
Oct 21, 2022
Merged

fix: Revert pfect args make iterator#4234
henryiii merged 3 commits into
pybind:masterfrom
Skylion007:revert-pfect-args-make-iterator

Conversation

@Skylion007

@Skylion007Skylion007 commented Oct 11, 2022•
edited by henryiii
Loading

Copy link
Copy Markdown
Collaborator

Description

Closes#4100 . Apparently, the perfect forwarding here can cause some issues with the wrong template being selected here. One option is to revert the relevant commit that this PR demonstrates, but I am not sure that is the best long term fix as the ambiguous / wrong overload can be easily be selected since the two templates for make_iterator are under-constrained.

Suggested changelog entry:

* Revert perfect forwarding for make_iterator. This broke at least one valid use case. May revist later.

@Skylion007
Skylion007 marked this pull request as ready for review October 11, 2022 18:56
@Skylion007
Skylion007 marked this pull request as draft October 11, 2022 18:58
rwgk
rwgk approved these changes Oct 11, 2022
View reviewed changes

@rwgkrwgk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test!

@Skylion007

Skylion007 commented Oct 11, 2022•
edited by rwgk
Loading

Copy link
Copy Markdown
CollaboratorAuthor

What do you think the best longterm fix for the caster is here?

Short answer: I don't know. This is very involved code and I'd need a bit of time to really get my head around all the details that come into play here, and most likely I'd need to experiment because 1st time through I hardly ever get anything right.

PR #3980 had no tests and no measurements. How do we know that any effort spent here is actually worthwhile?

My best guess: there is very little to be had here, even with a lot of effort. But I could be wrong.

I cannot afford spending more time thinking about this until there are tests of some sorts or measurements that promise a tangible gain.

My recommendation:

  • Let's roll this back because we evidently broke a valid use case.
  • Someone needs to do at least timings (locally roll forward).
  • We review those and decide what to do.

@rwgk

rwgk commented Oct 19, 2022

Copy link
Copy Markdown
Collaborator

@Skylion007@henryiii I think we need this in v2.10.1 & v2.11

Aaron: Is there anything still needed here? Could we please merge this?

@henryiiihenryiii mentioned this pull request Oct 20, 2022
Closed
@henryiii

henryiii commented Oct 21, 2022

Copy link
Copy Markdown
Collaborator

@Skylion007 could you undraft? I think this would be a good interim solution, maybe we can revisit before 2.11/2.12.

@Skylion007
Skylion007 marked this pull request as ready for review October 21, 2022 21:49
@henryiii
henryiii merged commit 17c1e27 into pybind:masterOct 21, 2022
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Oct 21, 2022
henryiii pushed a commit to henryiii/pybind11 that referenced this pull request Oct 21, 2022
@Skylion007@henryiii
fix: Revert pfect args make iterator (pybind#4234)
305c471
* Revert "chore: perfectly forward all make_iterator args (pybind#3980)"
This reverts commit 8da58da.
* Redo unrelated optimization in commit
* Add tests for ambiguous overloads
@Skylion007
Skylion007 deleted the revert-pfect-args-make-iterator branch October 22, 2022 16:33
@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Oct 31, 2022
@rwgkrwgk mentioned this pull request Feb 11, 2023
Closed
@henryiiihenryiii mentioned this pull request Jul 25, 2026
Closed
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rwgkrwgkrwgk approved these changes
@henryiiihenryiiihenryiii approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG]: error: lvalue required as increment operand

3 participants

@Skylion007@rwgk@henryiii