Set of small refactoring and fixes #1293

Merged
merged 5 commits into from Aug 6, 2013

Projects

None yet

8 participants

@Karlson2k
Member

While debugging of XBMC I found some points that need enhancement.
So here is my set on commits.

@wsoltys
Member
wsoltys commented Aug 16, 2012

The only fix I see is 607d320 and thanks for that. But for what are the other changes needed or do they fix?

@Karlson2k
Member

Other fixes in 00a033d .

  1. utf8ToW was used wrong (we don't need to reverse anything!)
  2. ANSI version of GetFullPathName will broke path in non-ANSI filesystem
  3. No actual check is GetFullPathName really return something or return was not fit in to buffer

1c14c3c , d74c6d9, 1f40cf0 replace big arrays in stack with CStdString (so alloc is used). Heavily using stack with 64 kilobyte array could potencially leads to stack overflow.

@wsoltys
Member
wsoltys commented Aug 16, 2012

sigh I'm not in the mood to argue here. Don't get me wrong I appreciate any help for the windows platform but couldn't you fix bugs mentioned on trac where users would benefit?
Concerning this pr I would be fine if you could replace CStdString/W by std:string (where appropriate) since you already touched it.
Just removing a workaround (c9f82ae) because it works for you isn't enough verifying for me. It was there for a reason and maybe one should speak with the original author.

@Karlson2k
Member

@wsoltys Commit removed.
I can't switch to std::string here as it does not provide method for direct buffer manipulation.

It's just a part of my understanding of XBMC. :) I just want to fix obvious things.

@jmarshallnz
Member

Just drop the bits that use CStdString(W) in place of the WCHAR buffer - there's no decent reason to do so. The others look fine.

@Karlson2k
Member

@jmarshallnz
Are there any defined limits for stack usage in XBMC?
I thought that placing anything more than 10k in heap instead of stack is good and right coding style as extensive stack usage could produce stack overflow errors that can be hardly identified.

@jmarshallnz
Member

The 32k WCHAR array is only 64k on win32, which should be OK - that one is probably a bit questionable though, so can probably be left the way you have it (it's already using a CStdStringW anyway).

The others don't really need changing from the array on the stack. We prefer not to use CStdString unless we have to as it's not as easily portable.

@Karlson2k
Member

@jmarshallnz
Isn't 32k too large?
Default stack size on Win32 is 1M. If 64k in stack is just fine, then we can have less that 15 steps deep calls of such functions. Moreover on OSX within non-main thread default stack size is 512k, so in case of 64k stack arrays we have limit of less that 7 calls.
Common recommendation is put everything more than a few kilobytes in heap.
VS code analyzer complains about any function that puts more that 16k in stack.
I think that if we can easily convert large variable from stack to heap, than we should do it. Am I wrong?
And I want to fix some limit about stack variable, which is OK for XBMC (in all platforms), and not to buzz about this topic again.

@jmarshallnz
Member

I've already said it's a bit on the large size. This is very much an exceptional case, though - I doubt you'll find any this large elsewhere in the codebase.

I have no problem with you changing this, just don't use CStdStringW unless you have no other option.

@wsoltys
Member
wsoltys commented Aug 26, 2012

I'm fine with CStdString for the MAX_PATH case rather than playing new/delete games there.

@wsoltys
Member
wsoltys commented Aug 26, 2012

Ah, mixed MAX_PATH with the 32k. You know what I wanted to express :)

@Karlson2k
Member

@wsoltys Yes, I got it. :)
@jmarshallnz About large stack usage... Check out:

  • CKaraokeLyricsCDG::cmdScroll, tmp
  • CZipFile::Seek, tmp
  • CSAPSessions::Process, data
  • CKaraokeLyricsCDG::Render(), pixelbuf

It's just some of them... :)

@jmarshallnz
Member

That's fine - so point those cases out, rather than silly things that use
MAX_PATH = 260.

On Mon, Aug 27, 2012 at 9:51 AM, Karlson2k notifications@github.com wrote:

@wsoltys https://github.com/wsoltys Yes, I got it. :)
@jmarshallnz https://github.com/jmarshallnz About large stack usage...
Check out:

  • CKaraokeLyricsCDG::cmdScroll, tmp
  • CZipFile::Seek, tmp
  • CSAPSessions::Process, data
  • CKaraokeLyricsCDG::Render(), pixelbuf

It's just some of them... :)


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1293#issuecomment-8038207.

@Karlson2k
Member

@jmarshallnz Yes, you are right.
OK, I'll try to concentrate on more serious bugs/problems. :)

@jmarshallnz
Member

@Karlson2k: Any chance you're going to clean this one up at all? If not, please close.

@Karlson2k
Member

I'll update it today.

@MartijnKaijser
Member

can you rebase?

@Karlson2k
Member

@MartijnKaijser rebased and updated

@MartijnKaijser
Member

jenkins build this please
@wsoltys any chance you could go through the commits if we want these changes/fixes?

@Memphiz
Member
Memphiz commented Aug 6, 2013

this pr is to old for jenkins i guess (can't login to it atm - i am getting error 500 - @MartijnKaijser can you login to jenkins?)

@MartijnKaijser
Member

not from work (stupid web block). shouldn't a rebase have solved this?

@Memphiz
Member
Memphiz commented Aug 6, 2013

No it is not in the index of jenkins - so it doesn't know anything about this PR (this happend to some old pull requests which it somehow didn't get during initial scan...)

@wsoltys
Member
wsoltys commented Aug 6, 2013

No idea about the isondvd() change. Otherwise it's fine.

@MartijnKaijser
Member

almost have the idea that that could be something that never used anymore or do we still run from DVD now days? wasn't that something from the XBMC live which we don't do anymore

@Memphiz
Member
Memphiz commented Aug 6, 2013

jenkins build this please (i can login on jenkins again and see in the logs that the github-api was down for maintenance which not only prevented me from logging in to jenkins - but also might have prevented the trigger to work out ... so i try again with this comment...)

@Karlson2k
Member

@wsoltys IsDVD checks for video DVD, IsOnDVD checks for anything located on DVD.
@MartijnKaijser Seems that no one complains about inability to run from DVD. I didn't check - may be XBMC can run from DVD, but with some errors on log.

@MartijnKaijser MartijnKaijser merged commit 457a7e8 into xbmc:master Aug 6, 2013

1 check passed

default Build #100 succeeded in 1 hr 28 min
Details
@wsoltys
Member
wsoltys commented Aug 6, 2013

This is a legacy from the Xbox days. Doubt that it has a use case today.

Am 06.08.2013 um 13:19 schrieb Karlson2k notifications@github.com:

@wsoltys IsDVD checks for video DVD, IsOnDVD checks for anything located on DVD.
@MartijnKaijser Seems that no one complains about inability to run from DVD. I didn't check - may be XBMC can run from DVD, but with some errors on log.


Reply to this email directly or view it on GitHub.

@ace20022
Member
ace20022 commented Aug 6, 2013

I get "CSettings: error loading settings definition from special://xbmc/system/settings/settings.xml, Line 0" upon start with this pr.

@Karlson2k
Member

@ace20022 Do you have non-latin letters in path to settings.xml?
Platform?

@Karlson2k
Member

@ace20022
Can't reproduce it with latin and non-latin paths, on Windows and on Ubuntu, with this branch and with master branch.
Could you provide a little bit more info?

@ace20022
Member
ace20022 commented Aug 6, 2013

@Karlson2k Sorry, was out till now, will investigate it in more detail tomorrow. Platform: Win 7, no non-latin chars.

@MartijnKaijser
Member

No issues here either build from your branch or from master (win7)

@Karlson2k
Member

@ace20022 No such errors on other branches?
@MartijnKaijser Thanks!

@Voyager1
Member
Voyager1 commented Aug 7, 2013

no issues here either (Win 8). Don't know if relevant but settings.xml is under C:\Program Files (x86)\XBMC\system\settings (my roaming C:\Users...\AppData\Roaming\XBMC\system folder is empty)

@Montellese
Member

@Voyager1: that's where it belongs. It's not meant to be modified by users.

@ace20022
Member
ace20022 commented Aug 7, 2013

Now i'm feeling really stupid. It was a trailing whitespace introduced when I added ActiveAE to the environment list.
With f38268e "CUtil::GetHomePath fixes ..." the space remained...

Sorry for the noise!

@Karlson2k
Member

@ace20022 Do you mean than function now works right and didn't work with you misconfigured environment?

@ace20022
Member
ace20022 commented Aug 7, 2013

Yup, the path was "foo ", before your commit it was trimmed to "foo". In sum it was an error on my side.

@Karlson2k Karlson2k deleted the Karlson2k:win32_fixes branch Oct 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment