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

Finally getting rid of CStdString #6121

Merged
merged 34 commits into from
Jan 7, 2015

Conversation

Montellese
Copy link
Member

I've looked through @jmarshallnz's branches and came across the last step in getting rid of CStdString once and for all (based on the work of @notspiff). So I cherry-picked all of the commits, fixed all conflicts and finally fixed the win32 build. I then abused jenkins to test build all the other platforms and now they all build fine on jenkins. I made sure that win32 still compiles after every commit but I wasn't able to do the same for linux. It should (TM) work there as well but it's not tested.

While handling the conflicts and fixing the platforms I came across two changes that I'm not 100% sure about:

  • b50008b works around the fact that you can't assign a char* to a std::wstring by assigning it to a std::string and then copying that into the std::wstring. Maybe there's a cleaner solution.
  • ff6b40d adds #include <stdio.h> to DllLibCurl.h because it failed to compile on linux due to FILE being undefined. The issue here is that curl/curl.h already includes stdio.h but we include curl/curl.h in the XCURL namespace (to avoid name collisions). But according to some responses in the GCC bug tracker this should not be done because including standard C headers in a namespace results in undefined behaviour. But we've used this for ages.

I'd like to get this in ASAP because it touches a lot of code and has a high chance for conflicts.

@Montellese
Copy link
Member Author

jenkins build this please

@Montellese Montellese added the Type: Cleanup non-breaking change which removes non-working or unmaintained functionality label Jan 6, 2015
@Montellese
Copy link
Member Author

All builds went fine except for RBPI (see http://jenkins.kodi.tv/job/BuildMulti-PR/2017/). I've pushed a fix and am running only that build now.

@Montellese
Copy link
Member Author

OK RBPI build was successful.
jenkins build this please

@Montellese Montellese added this to the Helix 15.0-alpha1 milestone Jan 6, 2015
@Montellese
Copy link
Member Author

If nobody objects until tomorrow I'll squash the two "COMMENT" into the respective commits and merge this.

@Memphiz
Copy link
Member

Memphiz commented Jan 6, 2015

imo this is a good approach - we will see any fall out in the forums i guess - similar to what we spotted after the first phases of those kind of PRs - to huge to review in a sane way imo.

@Jalle19
Copy link
Member

Jalle19 commented Jan 6, 2015

I guess we now really need to start working on doing the same for the PVR addons.

@notspiff
Copy link
Contributor

notspiff commented Jan 6, 2015

You will quickly realize you need stringutils.

@opdenkamp
Copy link
Member

using StdString or not for add-ons is up to whoever maintains the add-on. our repos is just a collection of whatever upstreams provide. except for the tvheadend and demo add-ons, feel free to rip it out of those two if it's used there. and then you're going to need stringutils in there too like spiff said.

@Montellese
Copy link
Member Author

Squashed the two COMMENT commits and rebased onto latest master.
jenkins build and merge

@Jalle19
Copy link
Member

Jalle19 commented Jan 7, 2015

@notspiff @opdenkamp replacing it with StringUtils was the idea.

@jenkins4kodi jenkins4kodi merged commit e7014bf into xbmc:master Jan 7, 2015
@Montellese Montellese deleted the stdstring_removal_part3 branch January 7, 2015 07:54
@notspiff
Copy link
Contributor

notspiff commented Jan 7, 2015

My plan was to put stringutils in kodi-platform then have kodi depend on that.

phil65 pushed a commit to phil65/xbmc that referenced this pull request Jan 10, 2015
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants