Skip to content
Permalink
Browse files

Fix some memory leaks identified by Coverity

Also improve the API for QgsExpressionContextUtils::updateSymbolScope
Previously it was hard to predict if the method would create storage,
which caused issues for the python bindings (eg, they had a choice of
either leaking or being crashy).
  • Loading branch information
nyalldawson committed Jan 24, 2016
1 parent 1dc8fc3 commit 8ad6ca08fe2556f787ef83cebaf11d93d16a25fa
@@ -345,9 +345,9 @@ class QgsExpressionContext
void appendScope( QgsExpressionContextScope* scope /Transfer/ );

/**
* Remove the last scope from the expression context.
* Removes the last scope from the expression context and return it.

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Jan 24, 2016

Member

Either do it all in passive voice or nothing ;)

*/
void popScope();
QgsExpressionContextScope* popScope();

This comment has been minimized.

Copy link
@wonder-sk

wonder-sk Jan 25, 2016

Member

@nyalldawson I think this method should have /TransferBack/ annotation as it moves the ownership from c++ to python


/** Appends a scope to the end of the context. This scope will override
* any matching variables or functions provided by existing scopes within the
@@ -491,14 +491,12 @@ class QgsExpressionContextUtils
static QgsExpressionContextScope* mapSettingsScope( const QgsMapSettings &mapSettings ) /Factory/;

/**
* Updates a symbol scope related to a QgsSymbolV2 to an expression context. If there is no existing scope
* provided, a new one will be generated and returned.
* Updates a symbol scope related to a QgsSymbolV2 to an expression context.
* @param symbol symbol to extract properties from
* @param symbolScope optional pointer to an existing scope to update
* @param symbolScope pointer to an existing scope to update
* @note added in QGIS 2.14
*/
static QgsExpressionContextScope* updateSymbolScope( const QgsSymbolV2* symbol, QgsExpressionContextScope* symbolScope = nullptr ) /Factory/;

static QgsExpressionContextScope* updateSymbolScope( const QgsSymbolV2* symbol, QgsExpressionContextScope* symbolScope = nullptr );

/** Creates a new scope which contains variables and functions relating to a QgsComposition.
* For instance, number of pages and page sizes.
@@ -50,7 +50,7 @@ static QgsExpressionContext _getExpressionContext( const void* context )
if ( layer )
expContext << QgsExpressionContextUtils::layerScope( layer );

expContext << QgsExpressionContextUtils::updateSymbolScope( nullptr );
expContext << QgsExpressionContextUtils::updateSymbolScope( nullptr, new QgsExpressionContextScope() );

//TODO - show actual value
expContext.setOriginalValueVariable( QVariant() );
@@ -384,10 +384,12 @@ void QgsExpressionContext::appendScope( QgsExpressionContextScope* scope )
mStack.append( scope );
}

void QgsExpressionContext::popScope()
QgsExpressionContextScope* QgsExpressionContext::popScope()
{
if ( !mStack.isEmpty() )
mStack.pop_back();
return mStack.takeLast();

return nullptr;
}

QgsExpressionContext& QgsExpressionContext::operator<<( QgsExpressionContextScope* scope )
@@ -721,7 +723,7 @@ QgsExpressionContextScope* QgsExpressionContextUtils::mapSettingsScope( const Qg
QgsExpressionContextScope* QgsExpressionContextUtils::updateSymbolScope( const QgsSymbolV2* symbol, QgsExpressionContextScope* symbolScope )
{
if ( !symbolScope )
symbolScope = new QgsExpressionContextScope( QObject::tr( "Symbol Scope" ) );
return nullptr;

symbolScope->addVariable( QgsExpressionContextScope::StaticVariable( QgsExpressionContext::EXPR_SYMBOL_COLOR, symbol ? symbol->color() : QColor(), true ) );

@@ -380,9 +380,9 @@ class CORE_EXPORT QgsExpressionContext
void appendScope( QgsExpressionContextScope* scope );

/**
* Remove the last scope from the expression context.
* Removes the last scope from the expression context and return it.
*/
void popScope();
QgsExpressionContextScope* popScope();

/** Appends a scope to the end of the context. This scope will override
* any matching variables or functions provided by existing scopes within the
@@ -523,15 +523,13 @@ class CORE_EXPORT QgsExpressionContextUtils
static QgsExpressionContextScope* mapSettingsScope( const QgsMapSettings &mapSettings );

/**
* Updates a symbol scope related to a QgsSymbolV2 to an expression context. If there is no existing scope
* provided, a new one will be generated and returned.
* Updates a symbol scope related to a QgsSymbolV2 to an expression context.
* @param symbol symbol to extract properties from
* @param symbolScope optional pointer to an existing scope to update
* @param symbolScope pointer to an existing scope to update
* @note added in QGIS 2.14
*/
static QgsExpressionContextScope* updateSymbolScope( const QgsSymbolV2* symbol, QgsExpressionContextScope* symbolScope = nullptr );


/** Creates a new scope which contains variables and functions relating to a QgsComposition.
* For instance, number of pages and page sizes.
* @param composition source composition
@@ -134,7 +134,7 @@ class QgsExpressionSorter
indexedFeatures.append( indexedFeature );
}

expressionContext->popScope();
delete expressionContext->popScope();

qSort( indexedFeatures.begin(), indexedFeatures.end(), *this );

@@ -281,7 +281,8 @@ QList<QgsLabelFeature*> QgsVectorLayerLabelProvider::labelFeatures( QgsRenderCon
request.setSubsetOfAttributes( attrNames, mFields );
QgsFeatureIterator fit = mSource->getFeatures( request );

QgsExpressionContextScope* symbolScope = nullptr;
QgsExpressionContextScope* symbolScope = new QgsExpressionContextScope();
ctx.expressionContext().appendScope( symbolScope );
QgsFeature fet;
while ( fit.nextFeature( fet ) )
{
@@ -297,16 +298,14 @@ QList<QgsLabelFeature*> QgsVectorLayerLabelProvider::labelFeatures( QgsRenderCon
if ( !symbols.isEmpty() )
{
symbolScope = QgsExpressionContextUtils::updateSymbolScope( symbols.at( 0 ), symbolScope );
if ( !ctx.expressionContext().scopes().contains( symbolScope ) )
ctx.expressionContext().appendScope( symbolScope );
}
}
ctx.expressionContext().setFeature( fet );
registerFeature( fet, ctx, obstacleGeometry.data() );
}

if ( ctx.expressionContext().lastScope() == symbolScope )
ctx.expressionContext().popScope();
delete ctx.expressionContext().popScope();

if ( mRenderer )
mRenderer->stopRender( ctx );
@@ -286,7 +286,7 @@ void QgsVectorLayerRenderer::setGeometryCachePointer( QgsGeometryCache* cache )

void QgsVectorLayerRenderer::drawRendererV2( QgsFeatureIterator& fit )
{
QgsExpressionContextScope* symbolScope = QgsExpressionContextUtils::updateSymbolScope( nullptr );
QgsExpressionContextScope* symbolScope = QgsExpressionContextUtils::updateSymbolScope( nullptr, new QgsExpressionContextScope() );
mContext.expressionContext().appendScope( symbolScope );

QgsFeature fet;
@@ -366,7 +366,7 @@ void QgsVectorLayerRenderer::drawRendererV2( QgsFeatureIterator& fit )
}
}

mContext.expressionContext().popScope();
delete mContext.expressionContext().popScope();

stopRendererV2( nullptr );
}
@@ -384,7 +384,7 @@ void QgsVectorLayerRenderer::drawRendererV2Levels( QgsFeatureIterator& fit )
selRenderer->startRender( mContext, mFields );
}

QgsExpressionContextScope* symbolScope = QgsExpressionContextUtils::updateSymbolScope( nullptr );
QgsExpressionContextScope* symbolScope = QgsExpressionContextUtils::updateSymbolScope( nullptr, new QgsExpressionContextScope() );
mContext.expressionContext().appendScope( symbolScope );

// 1. fetch features
@@ -398,7 +398,7 @@ void QgsVectorLayerRenderer::drawRendererV2Levels( QgsFeatureIterator& fit )
{
qDebug( "rendering stop!" );
stopRendererV2( selRenderer );
mContext.expressionContext().popScope();
delete mContext.expressionContext().popScope();
return;
}

@@ -460,7 +460,7 @@ void QgsVectorLayerRenderer::drawRendererV2Levels( QgsFeatureIterator& fit )
}
}

mContext.expressionContext().popScope();
delete mContext.expressionContext().popScope();

// find out the order
QgsSymbolV2LevelOrder levels;
@@ -441,7 +441,7 @@ void QgsSymbolV2::startRender( QgsRenderContext& context, const QgsFields* field

QgsSymbolV2RenderContext symbolContext( context, outputUnit(), mAlpha, false, mRenderHints, nullptr, fields, mapUnitScale() );

QgsExpressionContextScope* scope = QgsExpressionContextUtils::updateSymbolScope( this );
QgsExpressionContextScope* scope = QgsExpressionContextUtils::updateSymbolScope( this, new QgsExpressionContextScope() );

mSymbolRenderContext->setExpressionContextScope( scope );

@@ -713,10 +713,13 @@ void QgsSymbolV2::renderFeature( const QgsFeature& feature, QgsRenderContext& co
deleteSegmentizedGeometry = true;
}

context.expressionContext().appendScope( mSymbolRenderContext->expressionContextScope() );
QgsExpressionContextUtils::updateSymbolScope( this, mSymbolRenderContext->expressionContextScope() );
mSymbolRenderContext->expressionContextScope()->setVariable( "geometry_part_count", segmentizedGeometry->geometry()->partCount() );
mSymbolRenderContext->expressionContextScope()->setVariable( "geometry_part_num", 1 );
if ( mSymbolRenderContext->expressionContextScope() )
{
context.expressionContext().appendScope( mSymbolRenderContext->expressionContextScope() );
QgsExpressionContextUtils::updateSymbolScope( this, mSymbolRenderContext->expressionContextScope() );
mSymbolRenderContext->expressionContextScope()->setVariable( "geometry_part_count", segmentizedGeometry->geometry()->partCount() );
mSymbolRenderContext->expressionContextScope()->setVariable( "geometry_part_num", 1 );
}

switch ( QgsWKBTypes::flatType( segmentizedGeometry->geometry()->wkbType() ) )
{
@@ -48,6 +48,7 @@ Qgs25DRendererWidget::Qgs25DRendererWidget( QgsVectorLayer* layer, QgsStyleV2* s
QgsExpressionContextScope* scope = QgsExpressionContextUtils::layerScope( mLayer );
QVariant height = scope->variable( "qgis_25d_height" );
QVariant angle = scope->variable( "qgis_25d_angle" );
delete scope;

mHeightWidget->setField( height.isNull() ? "10" : height.toString() );
mAngleWidget->setValue( angle.isNull() ? 70 : angle.toDouble() );
@@ -523,9 +523,9 @@ int vtableBestIndex( sqlite3_vtab *pvtab, sqlite3_index_info* indexInfo )
break;
#ifdef SQLITE_INDEX_CONSTRAINT_LIKE
case SQLITE_INDEX_CONSTRAINT_LIKE:
#endif
expr += " LIKE ";
break;
#endif
default:
break;
}
@@ -295,7 +295,7 @@ void TestQgsExpressionContext::contextStack()
QVERIFY( !context.isReadOnly( "test" ) );

// Check scopes can be popped
context.popScope();
delete context.popScope();
QCOMPARE( scopes.length(), 2 );
QCOMPARE( scopes.at( 0 ), scope1 );
}

4 comments on commit 8ad6ca0

@nyalldawson

This comment has been minimized.

Copy link
Contributor Author

@nyalldawson nyalldawson replied Jan 24, 2016

@m-kuhn thoughts on this? The previously approach was a bit unpredictable, this should make it less so

@m-kuhn

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Jan 24, 2016

I would assume that popFoo deletes foo and takeFoo transfers ownership to the caller.

And I thought about renaming updateSymbolScope to just symbolScope. It would behave like every other fooBarScope method except for the option to provide an existing scope to update.

@m-kuhn

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Jan 24, 2016

Does that not work?

QgsExpressionContextScope* symbolScope( QgsSymbolV2* symbol, QgsExpressionContextScope* scope = nullptr /Transfer/ ) /TransferBack/;
@m-kuhn

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Jan 25, 2016

Updated comment based on 8ad6ca0#commitcomment-15646270

Please sign in to comment.
You can’t perform that action at this time.