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

[xbmc] Add missing include of <utility> #7975

Merged
merged 1 commit into from
Sep 19, 2015

Conversation

Paxxi
Copy link
Member

@Paxxi Paxxi commented Sep 6, 2015

This is a bit oppinionated, moved system includes to the bottom to try and avoid masking missing includes.

@Paxxi Paxxi added Type: Fix non-breaking change which fixes an issue Type: Cleanup non-breaking change which removes non-working or unmaintained functionality labels Sep 6, 2015
@Paxxi
Copy link
Member Author

Paxxi commented Sep 6, 2015

jenkins build this please

@Paxxi
Copy link
Member Author

Paxxi commented Sep 6, 2015

ok test again without cstdlib changes
jenkins build this please

@Montellese
Copy link
Member

I always keep system includes at the top trying to follow the C++ Google Coding Guidelines.

@Paxxi
Copy link
Member Author

Paxxi commented Sep 6, 2015

@Montellese I did that as well until recently. The issue with it that I can see is that it hides missing includes in the headers

Lib.h

std::string a;

Lib.cpp

#include <string>
#include "Lib.h"

a = "yada"; //builds fine

User.cpp

#include "Lib.h"

a = "yada"; //build error

@Paxxi
Copy link
Member Author

Paxxi commented Sep 6, 2015

Seems like the google guidelines sticks them somewhat in the middle
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Order_of_Includes

@Montellese
Copy link
Member

Yeah they put the include of the header belonging to the source file at the very top followed by C system includes, C++ system includes and then local includes.

@Paxxi
Copy link
Member Author

Paxxi commented Sep 7, 2015

That style is fine by me, I'll update this

@Paxxi Paxxi force-pushed the include_utility branch 2 times, most recently from 3f89466 to d3efae4 Compare September 18, 2015 19:40
@Paxxi
Copy link
Member Author

Paxxi commented Sep 18, 2015

so updated to google style include, broke pvr by reordering includes so there's some forward declarations and new includes added.
jenkins build and merge please

@Paxxi
Copy link
Member Author

Paxxi commented Sep 19, 2015

jenkins build this please

@Paxxi
Copy link
Member Author

Paxxi commented Sep 19, 2015

jenkins build this please

@Paxxi
Copy link
Member Author

Paxxi commented Sep 19, 2015

jenkins build and merge this please

This is a bit oppinionated, moved system includes to the bottom to try and avoid masking missing includes.
@Paxxi
Copy link
Member Author

Paxxi commented Sep 19, 2015

jenkins build and merge this please

@MartijnKaijser MartijnKaijser added this to the Jarvis 16.0-alpha3 milestone Sep 19, 2015
@jenkins4kodi jenkins4kodi merged commit 6e03b00 into xbmc:master Sep 19, 2015
@FernetMenta
Copy link
Contributor

you would do me favour if you refrained from doing cosmetics in core. it is really painful rebasing VideoPlayer on this.

@Paxxi
Copy link
Member Author

Paxxi commented Sep 20, 2015

sorry about that @FernetMenta I'm done with these major clean-ups now for a while so should hopefully not cause you any more pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants