CStdString removal part1 #4976

Merged
merged 20 commits into from Jul 4, 2014

Conversation

Projects
None yet
5 participants
Member

jmarshallnz commented Jul 2, 2014

Righto, round 1 of @notspiff's work. There's another larger one to come once this is in, which I may split further for review purposes. This one is mostly straight-forward. Assumptions made primarily are paths are case-sensitive (we use them as such all over the show).

The commits marked REVIEW have some detail that's not completely obvious - most of the rest are pretty obvious.

One thing I'm not sure about is whether URL protocol options, used mainly with CurlFile, but also a couple of other places are case-sensitive or not. They're certainly mixed-case. Obviously if we compare two that are of different cases as the entire path they'll be different but this possibly doesn't hurt. @elupus any idea?

Another thing is there's a settings option fillter that I've made case-sensitive, where perhaps it shouldn't be (the localizestrings commit) - @Montellese?

@Karlson2k Perhaps you could take a quick look through - as I say, the vast majority are not controversial: the goal here is to get rid of it, not to make things optimal or anything like that, so I haven't done that except where there was a very cheap win (hardly ever). Obviously in some cases we include CStdString in unrelated areas (particularly the include) to keep the impact down: It'll be all gone once the next series are in.

Member

jmarshallnz commented Jul 2, 2014

jenkins build this please

Owner

Montellese commented Jul 2, 2014

Concerning case-sensitivity in SettingOptionsRegionsFiller that shouldn't be an issue since those values are pre-defined in the langinfo.xml files and aren't touched/altered by the settings system.

Member

jmarshallnz commented Jul 3, 2014

I've done most fixes suggested. I've also done up a new series looking at moving URL compares away from string compares in #4980.

Member

jmarshallnz commented Jul 3, 2014

jenkins build this please

Owner

Montellese commented Jul 3, 2014

You missed an Equals in xbmc\cores\DllLoader\exports\emu_msvcrt.cpp on line 842.
Also looks like somethings wrong with the call to CUtil::ScanForExternalSubtitles() in OMXPlayer.cpp on line 722.

Member

jmarshallnz commented Jul 3, 2014

Yup - aware + remedied. Will rebase it up on top of the url protocol stuff once that's in.

Member

jmarshallnz commented Jul 3, 2014

rebased. jenkins build this please

Member

jmarshallnz commented Jul 3, 2014

jenkins build this please

Member

jmarshallnz commented Jul 3, 2014

jenkins build this please

notspiff and others added some commits Jun 23, 2014

@notspiff notspiff [stdstring] get rid of CStdString from SaveFileStateJob ed3aed9
@notspiff notspiff [stdstring] get rid of CStdString from a bunch of URIUtils methods th…
…at return a string reference.
c401cf5
@notspiff notspiff [stdstring] get rid of CStdString from utils/md5.cpp|h f0ca9b9
@notspiff notspiff [stdstring] get rid of CStdString in xbmc/XBDateTime
NOTE: RFC1123 means Month matching can be case-sensitive
35a2132
@notspiff notspiff [stdstring] get rid of CStdString in CDVDDemux::GetStreamCodecName b315d8d
Jonathan Marshall [stdstring] get rid of CStdString in CMusicDatabase::Get(Album|Artist…
…)Path
39fb80b
Jonathan Marshall [stdstring] get rid of CStdString in URIUtils::CreateArchivePath() 223a748
@notspiff notspiff [stdstring] get rid of CStdString in MultiPath(Directory|File) bf914f7
@notspiff notspiff [stdstring] get rid of CStdString in SpecialProtocol
NOTE: Paths are compared case-sensitive.
85ab0c7
@notspiff notspiff [stdstring] get rid of CStdString in StringUtils::WordToDigits, Remov…
…eCRLF, and Join
dfc0561
Jonathan Marshall [stdstring] get rid of CStdString in a bunch of CUtil functions 207a5a1
@notspiff notspiff [stdstring] get rid of CStdString in CGUIControlFactory::GetCondition…
…alVisibility
7a6f2cc
Jonathan Marshall [stdstring] get rid of CStdString in LocalizeStrings, LangInfo, LangC…
…odeExpander

NOTE: settings values are compared case-sensitive in SettingOptionsRegionFiller
a95a57e
Jonathan Marshall [stdstring] get rid of CStdString in CCurlFile's http get/post functions d40bb49
@notspiff notspiff [stdstring] get rid of CStdString in CGUIKeyboardFactory, CGUIWindow:…
…:OnEditChanged
2ccbc0e
Jonathan Marshall [stdstring] get rid of CStdString in CGUIListItem, CFileItem, CFileIt…
…emList
1ba45b9
@notspiff notspiff [stdstring] get rid of CStdString in xbmc/URL 513249e
@notspiff notspiff [stdstring] get rid of CStdString in GUIMediaWindow, and overridden f…
…unctions in children
5b4593e
@notspiff notspiff [stdstring] get rid of CStdString in IGUIContainer, IGUIControl and i…
…mplementations
ea855f8
Jonathan Marshall [curl/URL] Compare URL protocol options for CurlFile in lower-case. bf1f1a9
Member

jmarshallnz commented Jul 4, 2014

jenkins build this please

@jmarshallnz jmarshallnz added a commit that referenced this pull request Jul 4, 2014

@jmarshallnz jmarshallnz Merge pull request #4976 from jmarshallnz/stdstring_removal_part1
CStdString removal part1
51e4254

@jmarshallnz jmarshallnz merged commit 51e4254 into xbmc:master Jul 4, 2014

1 check passed

default Merged build #977 succeeded in 58 min
Details

jmarshallnz deleted the jmarshallnz:stdstring_removal_part1 branch Jul 4, 2014

Member

elupus commented Jul 5, 2014

Protocol option names are not case sensitive I think, but the values are
(they should match behavior of http uri options)
On Jul 2, 2014 9:14 AM, "jmarshallnz" notifications@github.com wrote:

Righto, round 1 of @notspiff https://github.com/notspiff's work.
There's another larger one to come once this is in, which I may split
further for review purposes. This one is mostly straight-forward.
Assumptions made primarily are paths are case-sensitive (we use them as
such all over the show).

The commits marked REVIEW have some detail that's not completely obvious -
most of the rest are pretty obvious.

One thing I'm not sure about is whether URL protocol options, used mainly
with CurlFile, but also a couple of other places are case-sensitive or not.
They're certainly mixed-case. Obviously if we compare two that are of
different cases as the entire path they'll be different but this possibly
doesn't hurt. @elupus https://github.com/elupus any idea?

Another thing is there's a settings option fillter that I've made
case-sensitive, where perhaps it shouldn't be (the localizestrings commit)

@Karlson2k https://github.com/Karlson2k Perhaps you could take a quick
look through - as I say, the vast majority are not controversial: the goal
here is to get rid of it, not to make things optimal or anything like that,
so I haven't done that except where there was a very cheap win (hardly
ever). Obviously in some cases we include CStdString in unrelated areas
(particularly the include) to keep the impact down: It'll be all gone once

the next series are in.

You can merge this Pull Request by running

git pull https://github.com/jmarshallnz/xbmc stdstring_removal_part1

Or view, comment on, or merge it at:

#4976
Commit Summary

  • [stdstring] get rid of CStdString from SaveFileStateJob
  • [stdstring] get rid of CStdString from utils/md5.cpp|h
  • [stdstring] get rid of CStdString from a bunch of URIUtils methods
    that return a string reference.
  • [stdstring] get rid of CStdString in xbmc/XBDateTime
  • [stdstring] get rid of CStdString in CDVDDemux::GetStreamCodecName
  • [stdstring] get rid of CStdString in
    CMusicDatabase::Get(Album|Artist)Path
  • [stdstring] get rid of CStdString in MultiPath(Directory|File)
  • [stdstring] get rid of CStdString in URIUtils::CreateArchivePath()
  • [stdstring] get rid of CStdString in StringUtils::WordToDigits,
    RemoveCRLF, and Join
  • [stdstring] get rid of CStdString in a bunch of CUtil functions
  • [stdstring] get rid of CStdString in SpecialProtocol
  • [stdstring] get rid of CStdString in
    CGUIControlFactory::GetConditionalVisibility
  • [stdstring] get rid of CStdString in LocalizeStrings REVIEW PLEASE
  • [stdstring] get rid of CStdString in CCurlFile's http get/post
    functions
  • [stdstring] get rid of CStdString in CGUIKeyboardFactory,
    CGUIWindow::OnEditChanged
  • [stdstring] get rid of CStdString in CGUIListItem, CFileItem,
    CFileItemList REVIEW PLEASE
  • [stdstring] get rid of CStdString in xbmc/URL
  • [stdstring] get rid of CStdString in GUIMediaWindow, and overridden
    functions in children
  • [stdstring] get rid of CStdString in IGUIContainer, IGUIControl and
    implementations
  • [curl/URL] CurlFile URL protocol options are case-insensitive???
    REVIEW PLEASE
  • [uriutils] use case-sensitive comparisons on protocols

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#4976.

MartijnKaijser added this to the Helix 14.0-alpha1 milestone Jul 16, 2014

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