Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[cstdstring] continue removing cstdstring #4229

Closed
wants to merge 24 commits into from

4 participants

@night199uk
Collaborator

This removes most of the rest of CStdString. Still a little WIP but mostly done.
There are still some unchecked conversions from const char * to std::string in here, which will cause runtime crashes. This is because const char * can be NULL, which can be converted to std::string but runtime checking will fail causing an exception. I've covered anything that breaks for me, but it will depend on skin/settings/files/nfos, etc. Would appreciate more help to spot / fix any that are broken. Most of them seem to be around XML parsing (std::string somestring = xxx->Attribute("blah")). Difficult ones are where xxx->Attribute is passed through a function parameter to an std::string arg, etc.

It should be pretty safe to demise the CStdString.h header after this, although I've not gone so far as to try it yet.

@night199uk
Collaborator

@jmarshallnz @Memphiz @wsoltys @davilla

Also, this is only tested for OSX. There are probably some areas I missed due to not having Linux/Windows #defines, if I get time I'll test on Linux as that's easy, Windows would be last for me so any help there greatly appreciated.

night199uk added some commits
@night199uk night199uk [cstdstring] get rid of direct buffer access to cstdstrings 8b59840
@night199uk night199uk [cstdstring] get rid of makereverse/collate/collatenocase/format rela…
…ted stuff (unused)
c8fa115
@night199uk night199uk [cstdstring] get rid of equals 2c3ed83
@night199uk night199uk [cstdstring] add files missing from the xcode project c961cab
@night199uk night199uk [cstdstring] add std namespace everywhere we'll need it c164feb
@night199uk night199uk [cstdstring] add format utils for working with std::strings a50ef18
@night199uk night199uk [cstdstring] modify windows to use a cstdstring constructor 4c36420
@night199uk night199uk [cstdstring] modify thread to use a cstdstring constructor 3aef1f6
@night199uk night199uk [cstdstring] rewrite a couple of cddb functions to use std::strings t…
…o avoid conflicts
dc65eec
@night199uk night199uk [cstdstring] rewrite an unrarxlib function to avoid conflicts later e548ff0
@night199uk night199uk [cstdstring] change a couple of smartplaylist functions to use string…
… instead of const char
4c45cba
@night199uk night199uk [cstdstring] rewrite functions to not use the reserved variable name …
…'string'
fd67640
@night199uk night199uk [cstdstring] rewrite a few functions that rely on checking cstdstring…
… for null
becf5dc
@night199uk night199uk [cstdstring] cuedocument uses old logging function de2abcb
@night199uk night199uk [cstdstring] don't rely on convertability to wchar 472812b
@night199uk night199uk [cstdstring] add c_str() everywhere that relies on implicit convertab…
…ility of cstdstring
dcbac2c
@night199uk night199uk [cstdstring] convert all cstdstringarrays to vector<string> b83aec7
@night199uk night199uk [cstdstring] convert all CStdStringW to std::wstring 010bdb5
@night199uk night199uk [cstdstring] replace CStdStringA with std::string b4cabaf
@night199uk night199uk [cstdstring] remove unused StringUtils::JoinString e16af6c
@night199uk
Collaborator

@Karlson2k there's also some wchar/unicode handling stuff in here that probably wants checking.

@jmarshallnz
Owner

I'm not going to look at it until after Gotham, but I'd strongly suggest splitting out into a PR series instead of attempting to get it in in one go. e.g. split out the .Equals() bit into it's own PR (the main thing left to drop, which requires a line-by-line careful review), split out any refactoring stuff into a separate PR and so on - keeping them small will help get each bit into mainline.

Regarding the setting from NULL, would one option be to remove the const char* constructor and use a separate static wrapper to generate a CStdString from the const char* for assignment. That way you can get it compiling again and we have an easily identified list to check. (It'll only work if any other constructors via std::string for example don't mess it up - you want compile-breakage to get them all).

@Karlson2k
Collaborator

@night199uk Completely agree with @jmarshallnz. 12k changed lines are too much for single PR.
Try to split it to several PRs, each one with clear goals (remove XX, replace XX).
This will help to review and help you to concentrate on smaller areas.

@night199uk
Collaborator

agreed, I can break apart a few of these (probably the first 3 can be separate) but the change will still be huge anyway just because of the scope of CStdString so this is never going to be pretty. But each of these changes is either a) small, or b) limited to a very specific purpose (e.g. c_str() addition where required, or a search/replace on CStdString with std::string). So a couple of the biggest of these only need cursory review as they're simply search/replace pretty much.

@jmarshallnz good point on the const char * constructor, I'll work through that and try and catch any there.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone
@MartijnKaijser

guess this PR is obsolete now?

@jmarshallnz
Owner

I'm guessing so - it would need quite some breaking up I think to be reviewable. I'm sure @night199uk can reopen if/when he gets a chance to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 18, 2014
  1. @night199uk
  2. @night199uk
  3. @night199uk
  4. @night199uk
  5. @night199uk
  6. @night199uk
  7. @night199uk
  8. @night199uk
  9. @night199uk
  10. @night199uk
  11. @night199uk
  12. @night199uk
  13. @night199uk
  14. @night199uk
  15. @night199uk
  16. @night199uk
  17. @night199uk
  18. @night199uk
  19. @night199uk
  20. @night199uk
  21. @night199uk

    [cstdstring] replace all CStdStrings with std::string

    night199uk authored
    remove a few functions that become duplicates after the change
  22. @night199uk
  23. @night199uk
  24. @night199uk
Something went wrong with that request. Please try again.