Skip to content
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

fix incorrect operator lookup for postincrement #4536

Merged
merged 11 commits into from
Sep 18, 2022
Merged

fix incorrect operator lookup for postincrement #4536

merged 11 commits into from
Sep 18, 2022

Conversation

maxbachmann
Copy link
Contributor

@maxbachmann maxbachmann commented Dec 26, 2021

Fixes #4535

@da-woods
Copy link
Contributor

In principle this might break some existing code (if they've decided to only wrap one of the operators). I recommend:

  1. Add a sentence or two to https://github.com/cython/cython/blob/master/docs/src/userguide/migrating_to_cy30.rst
  2. Would it be possible to customize the error message slightly to say something like "wrap c++ operator++(int)" just to help people identify what they need to change?

Point 2 is only if it's an easy change!

@maxbachmann
Copy link
Contributor Author

In principle this might break some existing code (if they've decided to only wrap one of the operators)

This breaks the current iterator implementation, which only specifies the preincrement operator. I added the postincrement operators in #4528.

Add a sentence or two to https://github.com/cython/cython/blob/master/docs/src/userguide/migrating_to_cy30.rst

I will update the migration guide

Would it be possible to customize the error message slightly to say something like "wrap c++ operator++(int)" just to help people identify what they need to change?

I updated the error message to:

Invalid operand type for '++'. Wrap Foo::operator++(int)
Invalid operand type for '++'. Wrap Foo::operator++()

@da-woods
Copy link
Contributor

Thanks - it's fairly minor breakage and should be easy for users to fix, but hopefully giving them a little hint will make it easier to work out what to do

@maxbachmann
Copy link
Contributor Author

maxbachmann commented Dec 27, 2021

I just noticed that this does not work for nonmember operators yet.

@maxbachmann
Copy link
Contributor Author

It would be good to fix #4539 first.

@scoder
Copy link
Contributor

scoder commented Dec 28, 2021

Would it be possible to customize the error message slightly to say something like "wrap c++ operator++(int)" just to help people identify what they need to change?

I updated the error message to:

Invalid operand type for '++'. Wrap Foo::operator++(int)
Invalid operand type for '++'. Wrap Foo::operator++()

It's not clear even to me what "wrap" means here without looking through the code. Did you mean to ask users to add these operators to their external C++ class declarations? Or to correct the existing declarations?

@maxbachmann
Copy link
Contributor Author

maxbachmann commented Dec 28, 2021

It's not clear even to me what "wrap" means here without looking through the code. Did you mean to ask users to add these operators to their external C++ class declarations? Or to correct the existing declarations?

I think I will update this to be closer to the error message of gcc, which gives the following errors in this case:

error: no 'operator++(int)' declared for postfix '++'
error: no match for 'operator++' (operand type is 'example')

@maxbachmann
Copy link
Contributor Author

@scoder @da-woods I completely forgot about this PR of mine. I updated the error message and added a paragraph to the Cython 3 migration guide. It would be great to get this in before Cython 3, since this is a least a slightly breaking change.

Copy link
Contributor

@da-woods da-woods left a comment

Choose a reason for hiding this comment

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

It would be good to fix #4539 first.

I think we're prepared to ignore this for now right?

This PR seems a much simpler fix, so is probably worth going in even without the full non member bug fix.

Cython/Compiler/ExprNodes.py Outdated Show resolved Hide resolved
Cython/Compiler/ExprNodes.py Outdated Show resolved Hide resolved
Cython/Compiler/ExprNodes.py Show resolved Hide resolved
docs/src/userguide/migrating_to_cy30.rst Outdated Show resolved Hide resolved
docs/src/userguide/migrating_to_cy30.rst Outdated Show resolved Hide resolved
docs/src/userguide/migrating_to_cy30.rst Outdated Show resolved Hide resolved
tests/errors/cpp_preincrement.pyx Outdated Show resolved Hide resolved
@maxbachmann
Copy link
Contributor Author

I think we're prepared to ignore this for now right?

Yes I would ignore this issue for now

maxbachmann and others added 7 commits September 18, 2022 02:43
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
@maxbachmann
Copy link
Contributor Author

@da-woods I applied the requested changes

@da-woods da-woods added this to the 3.0 milestone Sep 18, 2022
@da-woods da-woods merged commit e0dda45 into cython:master Sep 18, 2022
@da-woods
Copy link
Contributor

Thanks @maxbachmann

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] postincrement in C++ handled incorrectly
3 participants