Proposed Fixes for Various GCC Warnings

[monkeyman]

I've looked through all the warnings that GCC generates when compiling the current SVN for the linuxport with the -Wall flag set.

The following warnings can be fixed with very little effort:

GUIFont.cpp:51: warning: unused variable ‘nw’
GUIFont.cpp:51: warning: unused variable ‘nh’
GUIWindowVideoNav.cpp:1052: warning: unused variable ‘bDownload’
GUITextBox.cpp:483: warning: unused variable ‘size’
These variables look like they can be safely deleted.

FileCache.h:57:2: warning: no newline at end of file
Platinum/Source/Core/PltService.cpp:685:2: warning: no newline at end of file
Just add a newline at the end of the files.

VideoDatabase.cpp:3790: warning: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long unsigned int’
VideoDatabase.cpp:3881: warning: format ‘%d’ expects type ‘int’, but argument 4 has type ‘long unsigned int’
These should be probably be "%lu" instead of "%d".

XBMChttp.cpp:2350: warning: control reaches end of non-void function
It doesn't really, maybe just return 0 or something to shut up gcc?

In UnrarXlib:
file.cpp: In member function ‘Int64 File::Tell()’:
file.cpp:501: warning: unused variable ‘HighDist’
file.cpp:503: warning: unused variable ‘pos’
Both HighDist and pos should be commented out as well.

file.cpp:8: warning: ‘CreatedFiles’ defined but not used
This is only referenced from commented out code, it should be commented out as well.

filefn.cpp:111: warning: unused variable ‘sm’
filefn.cpp:112: warning: unused variable ‘sc’
filefn.cpp:113: warning: unused variable ‘sa’
sm, sc, and sa should be inside the _WIN_32 ifdef since they're only referenced from there.

Thread.cpp:97: warning: format ‘%lu’ expects type ‘long unsigned int’, but argument 4 has type ‘Uint32’
I think "%lu" should be "%d", but I'm not sure since it's a custom type.

pathfn.cpp:8: warning: unused variable ‘Found’
This should probably be commented out as well.

rar.cpp:349: warning: unused variable ‘TitleShown’
rar.cpp:346: warning: unused variable ‘TotalPackSize’
rar.cpp:346: warning: unused variable ‘TotalUnpSize’
rar.cpp:339: warning: unused variable ‘FileMatched’
rar.cpp:479: warning: unused variable ‘pPrev’
rar.cpp:480: warning: unused variable ‘TitleShown’
rar.cpp:473: warning: unused variable ‘FileMatched’
rar.cpp:475: warning: unused variable ‘TotalPackSize’
rar.cpp:475: warning: unused variable ‘TotalUnpSize’
None of these are ever used.

DVDOverlayRenderer.cpp:56: warning: unused variable ‘ey’
DVDOverlayRenderer.cpp:57: warning: unused variable ‘er’
Neither are mentioned again in the function.

winxml.cpp:157: warning: unused variable ‘bRefresh’
CDDARipper.cpp:204: warning: unused variable ‘iTrack’
Never used.

EncoderWav.cpp:75: warning: passing NULL to non-pointer argument 2 of ‘DWORD SetFilePointer(CXHandle*, LONG, LONG*, DWORD)’

SetFilePointer(m_hFile, NULL, NULL, FILE_BEGIN);
Should probably read:
SetFilePointer(m_hFile, 0, NULL, FILE_BEGIN);

These are all pretty trivial, so I doubt any developers will care that much about them. However, fixing/silencing these may make finding "real" warnings easier.

[spiff]

please,

speak diff or somebody has to do all the work you just did all over again

[monkeyman]

Yes. I'll generate some diffs and post them when I get a chance.

[Gamester17]

http://xboxmediacenter.com/wiki/HOW-TO_submit_a_patch
http://xboxmediacenter.com/wiki/Linux_port_project

Thanks

[jmarshall]

Thanks - just a note for whoever implements these: Most of them apply to trunk, so please apply them there first, or let me know when it's done and I'll merge them back.

[monkeyman]

Here's part of the patch. However, I can't get linuxport head to compile. Some of the xrandr stuff seems to have broken compilation.

Index: xbmc/VideoDatabase.cpp
================================================== =================
--- xbmc/VideoDatabase.cpp (revision 11096)
+++ xbmc/VideoDatabase.cpp (working copy)
@@ -3787,7 +3787,7 @@

unsigned int time = timeGetTime();
if (!m_pDS->query(strSQL.c_str())) return false;
- CLog::Log(LOGDEBUG, "%s - query took %d ms", __FUNCTION__, timeGetTime() - time); time = timeGetTime();
+ CLog::Log(LOGDEBUG, "%s - query took %lu ms", __FUNCTION__, timeGetTime() - time); time = timeGetTime();
int iRowsFound = m_pDS->num_rows();
if (iRowsFound == 0)
{
@@ -3878,7 +3878,7 @@
}
m_pDS->close();
}
- CLog::Log(LOGDEBUG, "%s item retrieval took %d ms", __FUNCTION__, timeGetTime() - time); time = timeGetTime();
+ CLog::Log(LOGDEBUG, "%s item retrieval took %lu ms", __FUNCTION__, timeGetTime() - time); time = timeGetTime();

// CLog::Log(LOGDEBUG, "%s Time: %d ms", timeGetTime() - time);
return true;
Index: xbmc/GUIWindowVideoNav.cpp
================================================== =================
--- xbmc/GUIWindowVideoNav.cpp (revision 11096)
+++ xbmc/GUIWindowVideoNav.cpp (working copy)
@@ -1049,7 +1049,6 @@
CDirectory::GetDirectory(tag.m_strPath,tbnItems,".tbn");^M
CStdString strExpression;^M
strExpression.Format("season[ ._-](0?%i)\\.tbn",m_vecItems[itemNumber]->GetVideoInfoTag()->m_iSeason);^M
- bool bDownload=true;^M
CRegExp reg;^M
if (reg.RegComp(strExpression.c_str()))^M
{^M


I'll upload the full patch to sourceforge once I can get svn to compile.

[monkeyman]

Here's the compile-time error, that I got:

GraphicContext.cpp: In member function ‘void CGraphicContext::SetVideoResolution(RESOLUTION&, BOOL, bool)’:
GraphicContext.cpp:571: error: ‘g_xrandr’ was not declared in this scope
make[1]: *** [GraphicContext.o] Error 1

[monkeyman]

It seems that when I install libxrandr-dev that the compilation error is fixed. Maybe these should be #ifdef'd out if not installed?

[monkeyman]

Now I get this error:

In file included from FileFactory.cpp:34:
FileMMS.h:5:24: error: libmms/mms.h: No such file or directory
FileMMS.h:25: error: ISO C++ forbids declaration of ‘mms_t’ with no type
FileMMS.h:25: error: expected ‘;’ before ‘*’ token


Any ideas? Thanks.

[monkeyman]

I'm sorry, I guess I should read the damn README file next time. Nevermind. I'm an idiot.

[tslayer]

Check the Readme.Linux file. Specifically the apt-get stuff you need. I believe libmms is now needed.

Edit: Disregard. Looks like we posted at the same time.

[d4rk]

SVN 11098+ will autodetect libmms, making it optional.

[monkeyman]

Thanks.

I've uploaded my patch to sourceforge: https://sourceforge.net/tracker/index.php?func=detail&aid=1858654&group_id=87054&atid=581840 (https://sourceforge.net/tracker/index.php?func=detail&aid=1858654&group_id=87054&atid=581840)

[monkeyman]

I've uploaded a second patch that fixes some other GCC warnings. Compilations are now nearly warning-free!

https://sourceforge.net/tracker/index.php?func=detail&aid=1859434&group_id=87054&atid=581840

[monkeyman]

Here's a large number of gcc warnings fixes when the linuxport source is compiled with the -Wextra and -pedantic flags.

The majority of these fixes involve removing extra commas and semicolons in .h files that are not ISO C++ compliant.

For example:
namespaces should not be followed by a ';'
the last element in an enumeration should not be followed by a ','

Additionally, there are a few (3-4) fixes for comparison between signed and unsigned variables. I tried to follow the intent of the original code when making changes. Nevertheless, these changes may need to be reviewed by an XBMC team member.

Finally, xbmc/linux/PlatformDefs.h contains a few anonymous structs. However, anonymous structs are NOT legal in ISO C++. I did not attempt to fix these because I fear that my changes may break something.

Overall, these changes should make it easier to compile the code under different compilers than gcc and MSVC.

https://sourceforge.net/tracker/index.php?func=detail&aid=1860228&group_id=87054&atid=581840

[monkeyman]

I uploaded another patch to sourceforge relating to uninitialized variables.

This patch fixes various stack (aka local or automatic) variables that may be uninitialized before their first read.

In all cases, I initialized them to a "safe" default value. This should help prevent unexpected behaviour when these variables are read for the first time.

https://sourceforge.net/tracker/index.php?func=detail&aid=1860973&group_id=87054&atid=581840

[yuvalt]

Committed. thanks a lot for all your patches, monkeyman!!

[monkeyman]

You're welcome. Just make sure jmarshall knows that the most recent patches can be backported to the trunk.

[monkeyman]

Here are some fixes for the last few warnings other than signed-unsigned comparison and constant string conversion.

I suggest that -Wno-write-strings should be appended to the debug flags variable in the Makefile. These warnings are not needed and they tend to clutter up output on newer versions of GCC when -Wall is specified.

Also, I'm not sure how to fix this warning:
DVDPlayerAudio.cpp: In member function 'int CDVDPlayerAudio::DecodeFrame(DVDAudioFrame&, bool)':
DVDPlayerAudio.cpp:338: warning: control reaches end of non-void function

One could simply return 0 at the end, but I don't know what the side effects of doing so are.

Otherwise, compilations should now be warning-free!

https://sourceforge.net/tracker/index.php?func=detail&aid=1861058&group_id=87054&atid=581840