[patch] check return value
Erik Hovland
erik at hovland.org
Fri Dec 1 22:08:46 UTC 2006
This batch of patches goes through all of the calls to functions that
return values but are not checked.
The patch has annotations so I won't go through them here. But please
give it a look and tell me what you think. It has easily the most
controversial changes to date since it can change behavior based on
return values. But it didn't seem to cause any problems for me so I
figured I would send it along.
Thanks
E
--
Erik Hovland
mail: erik AT hovland DOT org
web: http://hovland.org/
PGP/GPG public key available on request
-------------- next part --------------
Collection of patches that check the return of calls that were
previously unchecked.
src/dynamicmode.cpp
------------------
o The code does the regexp twice on the sql statement to find
the limit. The second time is unnecessary and it wasn't being
checked (not that it had to).
o Minor change to the comment to make it slightly more readable.
src/engine/xine/xine-engine.cpp
-------------------------------
o xine_config_lookup_entry() returns a value that should be checked.
src/mediadevice/generic/genericmediadevice.cpp
----------------------------------------------
o The code checks to see if the supported file types list is empty() but
doesn't check the return value. I am pretty sure the coder meant to
clear the list instead of check for emptiness. Changed the call to clear().
src/metadata/m4a/mp4audiosampleentry.cpp
----------------------------------------
o readSizeAndType returns an error code of either true or false, check it.
src/playlist.cpp
---------------
o call to checkFileStatus() is unchecked. Check it and report if things aren't right.
o This one is iffy, I removed a call farther down the line for the same thing.
There may be a reason why there are both calls. I couldn't tease it out so anyone
who wants to explain it to me deserves a pint.
src/playlistitem.cpp
--------------------
o Both calls to removeRef() aren't checked. It isn't a big deal so I just added
a warning report. But I figured that since the other callers did check I would
change these to do the same.
Signed-off-by: Erik Hovland <erik at hovland.org>
---
src/dynamicmode.cpp | 5 ++---
src/engine/xine/xine-engine.cpp | 5 ++++-
src/mediadevice/generic/genericmediadevice.cpp | 2 +-
src/metadata/m4a/mp4audiosampleentry.cpp | 3 ++-
src/playlist.cpp | 12 +++++-------
src/playlistitem.cpp | 7 +++++--
6 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/src/dynamicmode.cpp b/src/dynamicmode.cpp
index 63b38c2..57574cd 100644
--- a/src/dynamicmode.cpp
+++ b/src/dynamicmode.cpp
@@ -284,7 +284,7 @@ DEBUG_BLOCK
uint limit=0, offset=0;
QRegExp limitSearch( "LIMIT.*(\\d+).*OFFSET.*(\\d+)" );
- int findLocation = sql.find( limitSearch, false );
+ int findLocation = limitSearch.search( sql, 0 );
if( findLocation == -1 ) //not found, let's find out the higher limit the hard way
{
QString counting( sql );
@@ -295,8 +295,7 @@ DEBUG_BLOCK
limit = countingResult[0].toInt();
}
else
- { // There's a Limit, so we've got to respect it.
- limitSearch.search( sql );
+ { // There's a Limit, we have to respect it.
// capturedTexts() gives us the strings that were matched by each subexpression
offset = limitSearch.capturedTexts()[2].toInt();
limit = limitSearch.capturedTexts()[1].toInt();
diff --git a/src/engine/xine/xine-engine.cpp b/src/engine/xine/xine-engine.cpp
index 3e3d0cc..b658407 100644
--- a/src/engine/xine/xine-engine.cpp
+++ b/src/engine/xine/xine-engine.cpp
@@ -1250,7 +1250,10 @@ bool XineEngine::getAudioCDContents(cons
if (!device.isNull()) {
debug() << "xine-engine setting CD Device to: " << device << endl;
xine_cfg_entry_t config;
- xine_config_lookup_entry(m_xine, "input.cdda_device", &config);
+ if (!xine_config_lookup_entry(m_xine, "input.cdda_device", &config)) {
+ emit statusText(i18n("Failed CD device lookup in xine engine"));
+ return false;
+ }
config.str_value = (char *)device.latin1();
xine_config_update_entry(m_xine, &config);
}
diff --git a/src/mediadevice/generic/genericmediadevice.cpp b/src/mediadevice/generic/genericmediadevice.cpp
index 978d217..f62dda1 100644
--- a/src/mediadevice/generic/genericmediadevice.cpp
+++ b/src/mediadevice/generic/genericmediadevice.cpp
@@ -325,7 +325,7 @@ GenericMediaDevice::GenericMediaDevice()
m_songLocation = QString::null;
m_podcastLocation = QString::null;
- m_supportedFileTypes.empty();
+ m_supportedFileTypes.clear();
m_configDialog = 0;
diff --git a/src/metadata/m4a/mp4audiosampleentry.cpp b/src/metadata/m4a/mp4audiosampleentry.cpp
index 6b01fb7..e99ded8 100644
--- a/src/metadata/m4a/mp4audiosampleentry.cpp
+++ b/src/metadata/m4a/mp4audiosampleentry.cpp
@@ -69,7 +69,8 @@ void MP4::Mp4AudioSampleEntry::parseEntr
TagLib::MP4::Fourcc fourcc;
TagLib::uint esds_size;
- mp4file->readSizeAndType( esds_size, fourcc );
+ if (!mp4file->readSizeAndType( esds_size, fourcc ))
+ return;
// read esds' main parts
if( size()-48 > 0 )
diff --git a/src/playlist.cpp b/src/playlist.cpp
index 444177d..28b3289 100644
--- a/src/playlist.cpp
+++ b/src/playlist.cpp
@@ -1627,7 +1627,11 @@ Playlist::activate( QListViewItem *item
#define item static_cast<PlaylistItem*>(item)
- checkFileStatus( item );
+ if ( !checkFileStatus( item ) )
+ {
+ Amarok::StatusBar::instance()->shortMessage( i18n("Local file does not exist.") );
+ return;
+ }
if( dynamicMode() && !m_dynamicDirt && !Amarok::repeatTrack() )
{
@@ -1664,12 +1668,6 @@ Playlist::activate( QListViewItem *item
advanceDynamicTrack();
}
- if( !item->isFilestatusEnabled() )
- {
- Amarok::StatusBar::instance()->shortMessage( i18n("Local file does not exist.") );
- return;
- }
-
if( Amarok::entireAlbums() )
{
if( !item->nextInAlbum() )
diff --git a/src/playlistitem.cpp b/src/playlistitem.cpp
index 8da9b71..5738148 100644
--- a/src/playlistitem.cpp
+++ b/src/playlistitem.cpp
@@ -986,7 +986,9 @@ void PlaylistItem::derefAlbum()
m_album->refcount--;
if( !m_album->refcount )
{
- listView()->m_prevAlbums.removeRef( m_album );
+ if (!listView()->m_prevAlbums.removeRef( m_album ))
+ warning() << "Unable to remove album reference from "
+ << "listView.m_prevAlbums" << endl;
listView()->m_albums[artist_album()].remove( album() );
if( listView()->m_albums[artist_album()].isEmpty() )
listView()->m_albums.remove( artist_album() );
@@ -1026,7 +1028,8 @@ void PlaylistItem::decrementTotals()
{
const Q_INT64 prevTotal = m_album->total;
Q_INT64 total = m_album->total * m_album->tracks.count();
- m_album->tracks.removeRef( this );
+ if (!m_album->tracks.removeRef( this ))
+ warning() << "Unable to remove myself from m_album" << endl;
total -= totalIncrementAmount();
m_album->total = Q_INT64( double( total + 0.5 ) / m_album->tracks.count() );
if( listView()->m_prevAlbums.findRef( m_album ) == -1 )
More information about the Amarok
mailing list