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 weird behaviour of g++ 4.7.3-1ubuntu10 and implement sf::Rect<T>::contains(const Rect<T>&) #458

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@krofna

krofna commented Aug 30, 2013

Rationale is in commit messages.

Work around weird behaviour of g++ 4.7.3-1ubuntu10
g++ somehow assumes that result of adding two short unsigned ints is an int, effectively breaking compilation.
Error is /usr/local/include/SFML/Graphics/Rect.inl:81:41: error: no matching function for call to ‘min(const short unsigned int&, int)’
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 31, 2013

Member

I haven't reviewed it yet, but since you've put two unrelated things in the same pull request it is very unlikely to be accepted.

Member

LaurentGomila commented Aug 31, 2013

I haven't reviewed it yet, but since you've put two unrelated things in the same pull request it is very unlikely to be accepted.

@krofna

This comment has been minimized.

Show comment
Hide comment
@krofna

krofna Aug 31, 2013

@LaurentGomila Second one depends on first so I didn't know how to split it propertly. git allows you to take only one commit tho.

krofna commented Aug 31, 2013

@LaurentGomila Second one depends on first so I didn't know how to split it propertly. git allows you to take only one commit tho.

@krofna

This comment has been minimized.

Show comment
Hide comment
@krofna

krofna Aug 31, 2013

I installed g++ 4.8.1-9ubuntu1. Minimal code:

#include <SFML/Graphics/Rect.hpp>

int main()
{
    sf::Rect<short unsigned int> a;
    return a.contains((short unsigned int)1, (short unsigned int)2);
}

Error is somewhat fancier in 4.8:

In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl: In instantiation of ‘bool sf::Rect<T>::contains(T, T) const [with T = short unsigned int]’:
a.cpp:6:63:   required from here
/usr/local/include/SFML/Graphics/Rect.inl:81:41: error: no matching function for call to ‘min(const short unsigned int&, int)’
     T minX = std::min(left, left + width);
                                         ^
/usr/local/include/SFML/Graphics/Rect.inl:81:41: note: candidates are:
In file included from /usr/include/c++/4.8/algorithm:61:0,
                 from /usr/local/include/SFML/Graphics/Rect.hpp:32,
                 from a.cpp:1:
/usr/include/c++/4.8/bits/stl_algobase.h:193:5: note: template<class _Tp> const _Tp& std::min(const _Tp&, const _Tp&)
     min(const _Tp& __a, const _Tp& __b)
     ^
/usr/include/c++/4.8/bits/stl_algobase.h:193:5: note:   template argument deduction/substitution failed:
In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl:81:41: note:   deduced conflicting types for parameter ‘const _Tp’ (‘short unsigned int’ and ‘int’)
     T minX = std::min(left, left + width);
                                         ^
In file included from /usr/include/c++/4.8/algorithm:61:0,
                 from /usr/local/include/SFML/Graphics/Rect.hpp:32,
                 from a.cpp:1:
/usr/include/c++/4.8/bits/stl_algobase.h:239:5: note: template<class _Tp, class _Compare> const _Tp& std::min(const _Tp&, const _Tp&, _Compare)
     min(const _Tp& __a, const _Tp& __b, _Compare __comp)
     ^
/usr/include/c++/4.8/bits/stl_algobase.h:239:5: note:   template argument deduction/substitution failed:
In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl:81:41: note:   deduced conflicting types for parameter ‘const _Tp’ (‘short unsigned int’ and ‘int’)
     T minX = std::min(left, left + width);
                                         ^
kimir@kimir-laptop:~$ g++-4.8 a.cpp -Wfatal-errors
In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl: In instantiation of ‘bool sf::Rect<T>::contains(T, T) const [with T = short unsigned int]’:
a.cpp:6:63:   required from here
/usr/local/include/SFML/Graphics/Rect.inl:81:41: error: no matching function for call to ‘min(const short unsigned int&, int)’
     T minX = std::min(left, left + width);
                                         ^
compilation terminated due to -Wfatal-errors.

krofna commented Aug 31, 2013

I installed g++ 4.8.1-9ubuntu1. Minimal code:

#include <SFML/Graphics/Rect.hpp>

int main()
{
    sf::Rect<short unsigned int> a;
    return a.contains((short unsigned int)1, (short unsigned int)2);
}

Error is somewhat fancier in 4.8:

In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl: In instantiation of ‘bool sf::Rect<T>::contains(T, T) const [with T = short unsigned int]’:
a.cpp:6:63:   required from here
/usr/local/include/SFML/Graphics/Rect.inl:81:41: error: no matching function for call to ‘min(const short unsigned int&, int)’
     T minX = std::min(left, left + width);
                                         ^
/usr/local/include/SFML/Graphics/Rect.inl:81:41: note: candidates are:
In file included from /usr/include/c++/4.8/algorithm:61:0,
                 from /usr/local/include/SFML/Graphics/Rect.hpp:32,
                 from a.cpp:1:
/usr/include/c++/4.8/bits/stl_algobase.h:193:5: note: template<class _Tp> const _Tp& std::min(const _Tp&, const _Tp&)
     min(const _Tp& __a, const _Tp& __b)
     ^
/usr/include/c++/4.8/bits/stl_algobase.h:193:5: note:   template argument deduction/substitution failed:
In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl:81:41: note:   deduced conflicting types for parameter ‘const _Tp’ (‘short unsigned int’ and ‘int’)
     T minX = std::min(left, left + width);
                                         ^
In file included from /usr/include/c++/4.8/algorithm:61:0,
                 from /usr/local/include/SFML/Graphics/Rect.hpp:32,
                 from a.cpp:1:
/usr/include/c++/4.8/bits/stl_algobase.h:239:5: note: template<class _Tp, class _Compare> const _Tp& std::min(const _Tp&, const _Tp&, _Compare)
     min(const _Tp& __a, const _Tp& __b, _Compare __comp)
     ^
/usr/include/c++/4.8/bits/stl_algobase.h:239:5: note:   template argument deduction/substitution failed:
In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl:81:41: note:   deduced conflicting types for parameter ‘const _Tp’ (‘short unsigned int’ and ‘int’)
     T minX = std::min(left, left + width);
                                         ^
kimir@kimir-laptop:~$ g++-4.8 a.cpp -Wfatal-errors
In file included from /usr/local/include/SFML/Graphics/Rect.hpp:199:0,
                 from a.cpp:1:
/usr/local/include/SFML/Graphics/Rect.inl: In instantiation of ‘bool sf::Rect<T>::contains(T, T) const [with T = short unsigned int]’:
a.cpp:6:63:   required from here
/usr/local/include/SFML/Graphics/Rect.inl:81:41: error: no matching function for call to ‘min(const short unsigned int&, int)’
     T minX = std::min(left, left + width);
                                         ^
compilation terminated due to -Wfatal-errors.
@crumblingstatue

This comment has been minimized.

Show comment
Hide comment
@crumblingstatue

crumblingstatue Aug 31, 2013

I can reproduce this error with the test case provided by krofna.

GCC 4.8.1, SFML 2.1, Arch Linux

Can also reproduce with MinGW32 cross compiler (4.7.2).

According to this Stack Overflow answer: http://stackoverflow.com/a/7538579

(§ 4.5/1) - An rvalue of type char, signed char, unsigned char, short int, or unsigned short int can be converted to an rvalue of type int if int can represent all the values of the source type; otherwise, the source rvalue can be converted to an rvalue of type unsigned int.

http://en.cppreference.com/w/cpp/language/implicit_cast also talks about this.

crumblingstatue commented Aug 31, 2013

I can reproduce this error with the test case provided by krofna.

GCC 4.8.1, SFML 2.1, Arch Linux

Can also reproduce with MinGW32 cross compiler (4.7.2).

According to this Stack Overflow answer: http://stackoverflow.com/a/7538579

(§ 4.5/1) - An rvalue of type char, signed char, unsigned char, short int, or unsigned short int can be converted to an rvalue of type int if int can represent all the values of the source type; otherwise, the source rvalue can be converted to an rvalue of type unsigned int.

http://en.cppreference.com/w/cpp/language/implicit_cast also talks about this.

@krofna

This comment has been minimized.

Show comment
Hide comment
@krofna

krofna Aug 31, 2013

@crumblingstatue
Thanks for those links, looks like GCC is correct on this one.

krofna commented Aug 31, 2013

@crumblingstatue
Thanks for those links, looks like GCC is correct on this one.

@retep998

This comment has been minimized.

Show comment
Hide comment
@retep998

retep998 Aug 31, 2013

Also, you should be using static_cast, not a plain functional style cast.

retep998 commented Aug 31, 2013

Also, you should be using static_cast, not a plain functional style cast.

Implement bool sf::Rect::contains(const Rect<T>& rect) const;
This is somewhat faster and more convenient than checking if rect == intersection
@krofna

This comment has been minimized.

Show comment
Hide comment
@krofna

krofna Aug 31, 2013

@retep998
Right... I rebased & pushed it.

krofna commented Aug 31, 2013

@retep998
Right... I rebased & pushed it.

@ghost ghost assigned LaurentGomila Sep 14, 2013

LaurentGomila added a commit that referenced this pull request Oct 2, 2013

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 2, 2013

Member

Since you didn't split the request and I didn't want the contains function, I manually applied your fix.

Member

LaurentGomila commented Oct 2, 2013

Since you didn't split the request and I didn't want the contains function, I manually applied your fix.

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