[Karlson2k]

We are in process of CStdString removal from XBMC code.

As the rule: avoid using CStdString in new code as much as possible.

If you need to call function that still use CStdString as parameter or return type, check if std:tring can be automatically converted to/from CStdString.
If you are patching existing code, try to replace CStdString with std:tring at the same time (you don't have to do it if it forces you to update dozen other places).

[Memphiz]

ack

[jmarshall]

Note that I've updated night199uk's branch to latest master. I still need to review it carefully, but as soon as I'm happy with it (next couple days) I'll get it up and we can merge it before the next window, or at the end of the merge window. I don't mind which - am happy to rebase it up as new stuff hits (it's pretty easy now that I've done it once with git rere).

The approach is:

1. Leave CStdString in place (no need to alter function definitions).
2. Remove it's special functions. (new code can still use CStdString, but essentially you need to operate as if it's a std:tring).
3. Once all special functions are gone, change CStdString to a typedef to std:tring (no doubt there'll be more to cleanup once done).
4. Once that's done, replace all CStdString with std:tring via search+replace in the code (no doubt a few other issues creep in).
5. Cleanup the #includes.
6. Delete utils/StdString.h.

Note that I expect only some of step 2 will be done, but we'll see how I go.

Cheers,
Jonathan

[Karlson2k]

Once we have feature-freeze, I'll be open for boring fixes like this one.

[FernetMenta]

Once we have feature-freeze, I'll be open for boring fixes like this one.

I don't consider this as a fix. IMO the risk of breaking something is too high for doing such a big change after feature freeze.

[jmarshall]

Agreed, especially as there are some non-trivial things, such as CStdString quite happily allowing assignment to NULL, which std:tring won't like. Plus, CStdString also casts to const char * without .c_str(), which may hide some nasties.

With that said, I think a little progress will have been made by the end of the merge window. I have just about all routines cut out of StdString with the exception of .Equals() and a couple of other minor ones.

Cheers,
Jonathan

[Karlson2k]

jmarshall, add "#define SS_NO_IMPLICIT_CAST" between your points 2 and 3.

Since CStdString is using hacks to STL (see GetBuffer for example) and isn't bug-free, I consider this removal is bug-fixing.

[jmarshall]

Yes, I will disable SS_NO_IMPLICIT_CAST as it'll help find a bunch of places we should be using .c_str().

But there's other issues that make CStdString removal delicate. One obvious one is it accepts a NULL const char* in the constructor, whereas std:tring will crash. (simple usecase is TiXmlNode::GetValue vs TiXmlNode::GetValueStr() - we need to make sure we're using the latter) but I'm sure there's a bunch of others we assign to const char * blindly without first checking for null.

[Karlson2k]

Add
std:tring StringUtils::CreateStdStringWithCheck(const char* str)
?

[jmarshall]

Nasty. IMO we just fix it where we're doing it. Unfortunately this is much harder to detect as ofc std:tring has a const char * constructor.

(Most places I know of are when reading from XML, whereby there's a simple fix to use ValueStr() instead of Value()).

[jmarshall]

Ok, I've merged the initial CStdString removal work. It's quite neutered now, so proliferation should be kept to a minimum.

It took a LOT of (mostly) boring effort, and a lot of tedious but incredibly important review - thank you to those that did the work (night199uk) and those that helped with review (AlTheKiller, WiSo) - greatly appreciated. Also a shoutout to the jenkins guys. Without jenkins, I wouldn't have had a shot at getting it into shape for platforms I didn't have build systems up for.

There's 3 main things left:

1. Equals(). This is caseless by default in CStdString. I've made a start, and suggest others build off it here:

https://github.com/jmarshallnz/xbmc/tree/kill_equals

If you want to help out, please let me know so we can minimise duplicate effort. Please keep commits small and manageable, and clearly specify intent. For example, a commit that just handles cases where we compare against non-alpha characters can clearly be changed to == without much review effort being spent, as it isn't case-less. Don't make multiple changes that might be caseless or might not be in the same commit please. Anything you're not sure of, clearly mark in the commit message. I can always merge commits later, but splitting them up is no fun at all. Every single line will need careful review. Fortunately there's only a few thousand...

2. We'll need to remove the implicit cast to .c_str() and see what fallout that has. I suspect not a lot, as most of it will be in things like Format() which has already been taken care of.

3. We'll need to check very carefully for we're we are assigning (potentially) NULL const char*'s to CStdString's. It may be possible to track these down by explicitly removing the const char* constructor from CStdString. I expect there'll be a LOT of chaff to trawl through though...

All these will be post-Gotham unless specific fixes are discovered in the process, which I wouldn't at all be surprised at. If so, I'll pull them out individually.

Cheers,
Jonathan

[Memphiz]

Thx for taking that burden to all involved - i would die on such big stuff seriously...

[Montellese]

Yup thanks a lot for the work. According to the comments on github (and the fact that XBMC currently crashes on startup for me on win32) we also need to take a look at all the substr() calls introduced.

[jmarshall]

PR3654 should solve all (mayaswell be optimistic, right?) substr issues.

[arokh]

Sorry to hijack the thread guys, but I was wondering if any of you have time to look at spotyxbmc2?

https://github.com/JohNan/spotyxbmc2/commits/master

JohNan synced it to Gotham Alpha9, but since the CStdString changes it's been broken. Anybody have time and heart to properly sync it with master again?