Skip to content

Commit

Permalink
Avoid container detachments by using const methods wherever possible
Browse files Browse the repository at this point in the history 
eg QList::at() instead of QList:[], constFind instead of find, ...
  • Loading branch information
@nyalldawson
nyalldawson committed Dec 16, 2015
1 parent 6bfde51 commit 18614e1
Show file tree
Hide file tree
Showing 128 changed files with 680 additions and 768 deletions.
10 changes: 4 additions & 6 deletions src/analysis/interpolation/qgsidwinterpolator.cpp
View file
Expand Up @@ -47,18 +47,16 @@ int QgsIDWInterpolator::interpolatePoint( double x, double y, double& result )
double sumCounter = 0;
double sumDenominator = 0;

QVector<vertexData>::iterator vertex_it = mCachedBaseData.begin();

for ( ; vertex_it != mCachedBaseData.end(); ++vertex_it )
Q_FOREACH ( const vertexData& vertex_it, mCachedBaseData )
{
distance = sqrt(( vertex_it->x - x ) * ( vertex_it->x - x ) + ( vertex_it->y - y ) * ( vertex_it->y - y ) );
distance = sqrt(( vertex_it.x - x ) * ( vertex_it.x - x ) + ( vertex_it.y - y ) * ( vertex_it.y - y ) );
if (( distance - 0 ) < std::numeric_limits<double>::min() )
{
result = vertex_it->z;
result = vertex_it.z;
return 0;
}
currentWeight = 1 / ( pow( distance, mDistanceCoefficient ) );
sumCounter += ( currentWeight * vertex_it->z );
sumCounter += ( currentWeight * vertex_it.z );
sumDenominator += currentWeight;
}

Expand Down
18 changes: 8 additions & 10 deletions src/analysis/interpolation/qgsinterpolator.cpp
View file
Expand Up @@ -50,25 +50,23 @@ int QgsInterpolator::cacheBaseData()
mCachedBaseData.clear();
mCachedBaseData.reserve( 100000 );

QList<LayerData>::iterator v_it = mLayerData.begin();

for ( ; v_it != mLayerData.end(); ++v_it )
Q_FOREACH ( const LayerData& layer, mLayerData )
{
if ( v_it->vectorLayer == nullptr )
if ( !layer.vectorLayer )
{
continue;
}

QgsVectorLayer* vlayer = v_it->vectorLayer;
QgsVectorLayer* vlayer = layer.vectorLayer;
if ( !vlayer )
{
return 2;
}

QgsAttributeList attList;
if ( !v_it->zCoordInterpolation )
if ( !layer.zCoordInterpolation )
{
attList.push_back( v_it->interpolationAttribute );
attList.push_back( layer.interpolationAttribute );
}


Expand All @@ -80,9 +78,9 @@ int QgsInterpolator::cacheBaseData()
QgsFeature theFeature;
while ( fit.nextFeature( theFeature ) )
{
if ( !v_it->zCoordInterpolation )
if ( !layer.zCoordInterpolation )
{
QVariant attributeVariant = theFeature.attribute( v_it->interpolationAttribute );
QVariant attributeVariant = theFeature.attribute( layer.interpolationAttribute );
if ( !attributeVariant.isValid() ) //attribute not found, something must be wrong (e.g. NULL value)
{
continue;
Expand All @@ -94,7 +92,7 @@ int QgsInterpolator::cacheBaseData()
}
}

if ( addVerticesToCache( theFeature.constGeometry(), v_it->zCoordInterpolation, attributeValue ) != 0 )
if ( addVerticesToCache( theFeature.constGeometry(), layer.zCoordInterpolation, attributeValue ) != 0 )
{
return 3;
}
Expand Down
20 changes: 9 additions & 11 deletions src/analysis/interpolation/qgstininterpolator.cpp
View file
Expand Up @@ -84,12 +84,11 @@ void QgsTINInterpolator::initialize()
int nProcessedFeatures = 0;
if ( mShowProgressDialog )
{
QList<LayerData>::iterator layerDataIt = mLayerData.begin();
for ( ; layerDataIt != mLayerData.end(); ++layerDataIt )
Q_FOREACH ( const LayerData& layer, mLayerData )
{
if ( layerDataIt->vectorLayer )
if ( layer.vectorLayer )
{
nFeatures += layerDataIt->vectorLayer->featureCount();
nFeatures += layer.vectorLayer->featureCount();
}
}
}
Expand All @@ -103,18 +102,17 @@ void QgsTINInterpolator::initialize()


QgsFeature f;
QList<LayerData>::iterator layerDataIt = mLayerData.begin();
for ( ; layerDataIt != mLayerData.end(); ++layerDataIt )
Q_FOREACH ( const LayerData& layer, mLayerData )
{
if ( layerDataIt->vectorLayer )
if ( layer.vectorLayer )
{
QgsAttributeList attList;
if ( !layerDataIt->zCoordInterpolation )
if ( !layer.zCoordInterpolation )
{
attList.push_back( layerDataIt->interpolationAttribute );
attList.push_back( layer.interpolationAttribute );
}

QgsFeatureIterator fit = layerDataIt->vectorLayer->getFeatures( QgsFeatureRequest().setSubsetOfAttributes( attList ) );
QgsFeatureIterator fit = layer.vectorLayer->getFeatures( QgsFeatureRequest().setSubsetOfAttributes( attList ) );

while ( fit.nextFeature( f ) )
{
Expand All @@ -126,7 +124,7 @@ void QgsTINInterpolator::initialize()
}
theProgressDialog->setValue( nProcessedFeatures );
}
insertData( &f, layerDataIt->zCoordInterpolation, layerDataIt->interpolationAttribute, layerDataIt->mInputType );
insertData( &f, layer.zCoordInterpolation, layer.interpolationAttribute, layer.mInputType );
++nProcessedFeatures;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/analysis/vector/qgszonalstatistics.cpp
View file
Expand Up @@ -319,11 +319,11 @@ int QgsZonalStatistics::calculateStatistics( QProgressDialog* p )
double medianValue;
if ( even )
{
medianValue = ( featureStats.values[size / 2 - 1] + featureStats.values[size / 2] ) / 2;
medianValue = ( featureStats.values.at( size / 2 - 1 ) + featureStats.values.at( size / 2 ) ) / 2;
}
else //odd
{
medianValue = featureStats.values[( size + 1 ) / 2 - 1];
medianValue = featureStats.values.at(( size + 1 ) / 2 - 1 );
}
changeAttributeMap.insert( medianIndex, QVariant( medianValue ) );
}
Expand Down
30 changes: 13 additions & 17 deletions src/app/composer/qgscomposer.cpp
View file
Expand Up @@ -912,7 +912,7 @@ void QgsComposer::updateStatusZoom()

void QgsComposer::statusZoomCombo_currentIndexChanged( int index )
{
double selectedZoom = mStatusZoomLevelsList[ mStatusZoomLevelsList.count() - index - 1 ];
double selectedZoom = mStatusZoomLevelsList.at( mStatusZoomLevelsList.count() - index - 1 );
if ( mView )
{
mView->setZoomLevel( selectedZoom );
Expand Down Expand Up @@ -955,7 +955,7 @@ void QgsComposer::showItemOptions( QgsComposerItem* item )
return;
}

QMap<QgsComposerItem*, QWidget*>::iterator it = mItemWidgetMap.find( item );
QMap<QgsComposerItem*, QWidget*>::const_iterator it = mItemWidgetMap.constFind( item );
if ( it == mItemWidgetMap.constEnd() )
{
return;
Expand Down Expand Up @@ -2383,13 +2383,13 @@ struct QgsItemTempHider
}
void hideAll()
{
QgsItemVisibilityHash::iterator it = mItemVisibility.begin();
for ( ; it != mItemVisibility.end(); ++it ) it.key()->hide();
QgsItemVisibilityHash::const_iterator it = mItemVisibility.constBegin();
for ( ; it != mItemVisibility.constEnd(); ++it ) it.key()->hide();
}
~QgsItemTempHider()
{
QgsItemVisibilityHash::iterator it = mItemVisibility.begin();
for ( ; it != mItemVisibility.end(); ++it )
QgsItemVisibilityHash::const_iterator it = mItemVisibility.constBegin();
for ( ; it != mItemVisibility.constEnd(); ++it )
{
it.key()->setVisible( it.value() );
}
Expand Down Expand Up @@ -3426,8 +3426,8 @@ void QgsComposer::writeXML( QDomNode& parentNode, QDomDocument& doc )
composerElem.setAttribute( "title", mTitle );

//change preview mode of minimised / hidden maps before saving XML (show contents only on demand)
QMap< QgsComposerMap*, int >::iterator mapIt = mMapsToRestore.begin();
for ( ; mapIt != mMapsToRestore.end(); ++mapIt )
QMap< QgsComposerMap*, int >::const_iterator mapIt = mMapsToRestore.constBegin();
for ( ; mapIt != mMapsToRestore.constEnd(); ++mapIt )
{
mapIt.key()->setPreviewMode(( QgsComposerMap::PreviewMode )( mapIt.value() ) );
}
Expand Down Expand Up @@ -3629,11 +3629,7 @@ void QgsComposer::restoreGridSettings()
void QgsComposer::deleteItemWidgets()
{
//delete all the items
QMap<QgsComposerItem*, QWidget*>::iterator it = mItemWidgetMap.begin();
for ( ; it != mItemWidgetMap.end(); ++it )
{
delete it.value();
}
qDeleteAll( mItemWidgetMap );
mItemWidgetMap.clear();
}

Expand Down Expand Up @@ -3749,9 +3745,9 @@ void QgsComposer::addComposerHtmlFrame( QgsComposerHtml* html, QgsComposerFrame*

void QgsComposer::deleteItem( QgsComposerItem* item )
{
QMap<QgsComposerItem*, QWidget*>::iterator it = mItemWidgetMap.find( item );
QMap<QgsComposerItem*, QWidget*>::const_iterator it = mItemWidgetMap.constFind( item );

if ( it == mItemWidgetMap.end() )
if ( it == mItemWidgetMap.constEnd() )
{
return;
}
Expand Down Expand Up @@ -3937,8 +3933,8 @@ void QgsComposer::restoreComposerMapStates()
if ( !mMapsToRestore.isEmpty() )
{
//go through maps and restore original preview modes (show on demand after loading from project file)
QMap< QgsComposerMap*, int >::iterator mapIt = mMapsToRestore.begin();
for ( ; mapIt != mMapsToRestore.end(); ++mapIt )
QMap< QgsComposerMap*, int >::const_iterator mapIt = mMapsToRestore.constBegin();
for ( ; mapIt != mMapsToRestore.constEnd(); ++mapIt )
{
mapIt.key()->setPreviewMode(( QgsComposerMap::PreviewMode )( mapIt.value() ) );
mapIt.key()->cache();
Expand Down
2 changes: 1 addition & 1 deletion src/app/composer/qgscomposerlegendlayersdialog.cpp
View file
Expand Up @@ -46,7 +46,7 @@ QgsMapLayer* QgsComposerLegendLayersDialog::selectedLayer()
return nullptr;
}

QMap<QListWidgetItem*, QgsMapLayer*>::iterator it = mItemLayerMap.find( item );
QMap<QListWidgetItem*, QgsMapLayer*>::const_iterator it = mItemLayerMap.constFind( item );
QgsMapLayer* c = nullptr;
c = it.value();
return c;
Expand Down
22 changes: 11 additions & 11 deletions src/app/composer/qgscomposermanager.cpp
View file
Expand Up @@ -94,8 +94,8 @@ void QgsComposerManager::refreshComposers()
QSet<QgsComposer *> selectedComposers;
Q_FOREACH ( QListWidgetItem* item, mComposerListWidget->selectedItems() )
{
QMap<QListWidgetItem*, QgsComposer*>::iterator it = mItemComposerMap.find( item );
if ( it != mItemComposerMap.end() )
QMap<QListWidgetItem*, QgsComposer*>::const_iterator it = mItemComposerMap.constFind( item );
if ( it != mItemComposerMap.constEnd() )
{
selectedComposers << it.value();
}
Expand Down Expand Up @@ -414,8 +414,8 @@ void QgsComposerManager::remove_clicked()
// Find the QgsComposers that need to be deleted
Q_FOREACH ( QListWidgetItem* item, composerItems )
{
QMap<QListWidgetItem*, QgsComposer*>::iterator it = mItemComposerMap.find( item );
if ( it != mItemComposerMap.end() )
QMap<QListWidgetItem*, QgsComposer*>::const_iterator it = mItemComposerMap.constFind( item );
if ( it != mItemComposerMap.constEnd() )
{
composerList << it.value();
}
Expand All @@ -432,7 +432,7 @@ void QgsComposerManager::show_clicked()
{
Q_FOREACH ( QListWidgetItem* item, mComposerListWidget->selectedItems() )
{
QMap<QListWidgetItem*, QgsComposer*>::const_iterator it = mItemComposerMap.find( item );
QMap<QListWidgetItem*, QgsComposer*>::const_iterator it = mItemComposerMap.constFind( item );
if ( it != mItemComposerMap.constEnd() )
{
QgsComposer* c = nullptr;
Expand Down Expand Up @@ -468,8 +468,8 @@ void QgsComposerManager::duplicate_clicked()
QString currentTitle;

QListWidgetItem* item = mComposerListWidget->selectedItems().at( 0 );
QMap<QListWidgetItem*, QgsComposer*>::iterator it = mItemComposerMap.find( item );
if ( it != mItemComposerMap.end() )
QMap<QListWidgetItem*, QgsComposer*>::const_iterator it = mItemComposerMap.constFind( item );
if ( it != mItemComposerMap.constEnd() )
{
currentComposer = it.value();
currentTitle = it.value()->title();
Expand Down Expand Up @@ -519,8 +519,8 @@ void QgsComposerManager::rename_clicked()
QgsComposer* currentComposer = nullptr;

QListWidgetItem* item = mComposerListWidget->selectedItems().at( 0 );
QMap<QListWidgetItem*, QgsComposer*>::iterator it = mItemComposerMap.find( item );
if ( it != mItemComposerMap.end() )
QMap<QListWidgetItem*, QgsComposer*>::const_iterator it = mItemComposerMap.constFind( item );
if ( it != mItemComposerMap.constEnd() )
{
currentComposer = it.value();
currentTitle = it.value()->title();
Expand All @@ -543,8 +543,8 @@ void QgsComposerManager::rename_clicked()

void QgsComposerManager::on_mComposerListWidget_itemChanged( QListWidgetItem * item )
{
QMap<QListWidgetItem*, QgsComposer*>::iterator it = mItemComposerMap.find( item );
if ( it != mItemComposerMap.end() )
QMap<QListWidgetItem*, QgsComposer*>::const_iterator it = mItemComposerMap.constFind( item );
if ( it != mItemComposerMap.constEnd() )
{
it.value()->setTitle( item->text() );
}
Expand Down
4 changes: 2 additions & 2 deletions src/app/composer/qgscompositionwidget.cpp
View file
Expand Up @@ -460,8 +460,8 @@ void QgsCompositionWidget::applyCurrentPaperSettings()
if ( mComposition )
{
//find entry in mPaper map to set width and height
QMap<QString, QgsCompositionPaper>::iterator it = mPaperMap.find( mPaperSizeComboBox->currentText() );
if ( it == mPaperMap.end() )
QMap<QString, QgsCompositionPaper>::const_iterator it = mPaperMap.constFind( mPaperSizeComboBox->currentText() );
if ( it == mPaperMap.constEnd() )
{
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/app/gps/qgsgpsinformationwidget.cpp
View file
Expand Up @@ -880,7 +880,7 @@ void QgsGPSInformationWidget::on_mBtnCloseFeature_clicked()
memcpy( &wkb[1+sizeof( int )], &length, sizeof( int ) );
int position = 1 + 2 * sizeof( int );
double x, y;
for ( QList<QgsPoint>::iterator it = mCaptureList.begin(); it != mCaptureList.end(); ++it )
for ( QList<QgsPoint>::const_iterator it = mCaptureList.constBegin(); it != mCaptureList.constEnd(); ++it )
{
QgsPoint savePoint = *it;
// transform the gps point into the layer crs
Expand Down
14 changes: 7 additions & 7 deletions src/app/nodetool/qgsselectedfeature.cpp
View file
Expand Up @@ -249,13 +249,13 @@ void QgsSelectedFeature::deleteSelectedVertexes()
int count = 0;
for ( int i = mVertexMap.size() - 1; i > -1 && nSelected > 0; i-- )
{
if ( mVertexMap[i]->isSelected() )
if ( mVertexMap.at( i )->isSelected() )
{
if ( topologicalEditing )
{
// snap from current vertex
currentResultList.clear();
mVlayer->snapWithContext( mVertexMap[i]->pointV1(), ZERO_TOLERANCE, currentResultList, QgsSnapper::SnapToVertex );
mVlayer->snapWithContext( mVertexMap.at( i )->pointV1(), ZERO_TOLERANCE, currentResultList, QgsSnapper::SnapToVertex );
}

// only last update should trigger the geometry update
Expand Down Expand Up @@ -385,7 +385,7 @@ void QgsSelectedFeature::deleteVertexMap()

bool QgsSelectedFeature::isSelected( int vertexNr )
{
return mVertexMap[vertexNr]->isSelected();
return mVertexMap.at( vertexNr )->isSelected();
}

QgsGeometry *QgsSelectedFeature::geometry()
Expand Down Expand Up @@ -427,7 +427,7 @@ void QgsSelectedFeature::selectVertex( int vertexNr )
if ( vertexNr < 0 || vertexNr >= mVertexMap.size() )
return;

QgsVertexEntry *entry = mVertexMap[vertexNr];
QgsVertexEntry *entry = mVertexMap.at( vertexNr );
entry->setSelected();

emit selectionChanged();
Expand All @@ -438,7 +438,7 @@ void QgsSelectedFeature::deselectVertex( int vertexNr )
if ( vertexNr < 0 || vertexNr >= mVertexMap.size() )

This comment has been minimized.

Sign in to view

Copy link
@m-kuhn

m-kuhn Dec 16, 2015

Member

Wouldn't it be nicer to use value() For places where a check previous to .at() is done with a subsequent if( entry ) return;?
It's easier to read and we're sure not to forget the i < 0 part.
Mainly asking for your opinion, not asking you to change it

This comment has been minimized.

Sign in to view

Copy link
@nyalldawson

nyalldawson Dec 16, 2015

Author Collaborator

I actually don't like value() much. I lean toward the explicit checks as it makes it obvious what the behavior is for out of range indexes, and forces me to think about what the logical outcome should be in this scenario. It also makes it easier to debug, as the crash will occur right where the out of bounds access happens rather than somewhere else crashing because a null pointer is de-referenced. I also don't find .value()'s behaviour to be very "readable", as it doesn't match this standard c++ behaviour and may trip up devs who are new to Qt.

This comment has been minimized.

Sign in to view

Copy link
@m-kuhn

m-kuhn Dec 16, 2015

Member

It also makes it easier to debug, as the crash will occur right where the out of bounds access happens...

... if run on a developer's computer, if not a user will loose data since his last save. But admitted that also happens if a nullptr is dereferenced.

I like .value() because it's much faster to write than the check and I'm lazy. It also avoids off-by-one errors (i.e. writing i > size() instead of i >= size())

I recently found this:
b319435#diff-b3a61e32c4813473839a855b5cd6fd8aL85

Apparently nobody has called it with n<0 before but if we assume this we can as well assume the upper bound to be guaranteed.

And I don't like duplicated code ;)
return;

QgsVertexEntry *entry = mVertexMap[vertexNr];
QgsVertexEntry *entry = mVertexMap.at( vertexNr );
entry->setSelected( false );

emit selectionChanged();
Expand All @@ -448,7 +448,7 @@ void QgsSelectedFeature::deselectAllVertexes()
{
for ( int i = 0; i < mVertexMap.size(); i++ )
{
mVertexMap[i]->setSelected( false );
mVertexMap.at( i )->setSelected( false );
}
emit selectionChanged();
}
Expand All @@ -458,7 +458,7 @@ void QgsSelectedFeature::invertVertexSelection( int vertexNr )
if ( vertexNr < 0 || vertexNr >= mVertexMap.size() )
return;

QgsVertexEntry *entry = mVertexMap[vertexNr];
QgsVertexEntry *entry = mVertexMap.at( vertexNr );

bool selected = !entry->isSelected();

Expand Down

1 comment on commit 18614e1

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 18614e1 Dec 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

Please sign in to comment.