Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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 8ad6ca0
Show file tree
Hide file tree
Showing 11 changed files with 35 additions and 34 deletions.
12 changes: 5 additions & 7 deletions python/core/qgsexpressioncontext.sip
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/app/qgslabelinggui.cpp
Expand Up @@ -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() );
Expand Down
8 changes: 5 additions & 3 deletions src/core/qgsexpressioncontext.cpp
Expand Up @@ -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 )
Expand Down Expand Up @@ -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 ) );

Expand Down
10 changes: 4 additions & 6 deletions src/core/qgsexpressioncontext.h
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/core/qgsexpressionsorter.h
Expand Up @@ -134,7 +134,7 @@ class QgsExpressionSorter
indexedFeatures.append( indexedFeature );
}

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

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

Expand Down
7 changes: 3 additions & 4 deletions src/core/qgsvectorlayerlabelprovider.cpp
Expand Up @@ -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 ) )
{
Expand All @@ -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 );
Expand Down
10 changes: 5 additions & 5 deletions src/core/qgsvectorlayerrenderer.cpp
Expand Up @@ -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;
Expand Down Expand Up @@ -366,7 +366,7 @@ void QgsVectorLayerRenderer::drawRendererV2( QgsFeatureIterator& fit )
}
}

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

stopRendererV2( nullptr );
}
Expand All @@ -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
Expand All @@ -398,7 +398,7 @@ void QgsVectorLayerRenderer::drawRendererV2Levels( QgsFeatureIterator& fit )
{
qDebug( "rendering stop!" );
stopRendererV2( selRenderer );
mContext.expressionContext().popScope();
delete mContext.expressionContext().popScope();
return;
}

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

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

// find out the order
QgsSymbolV2LevelOrder levels;
Expand Down
13 changes: 8 additions & 5 deletions src/core/symbology-ng/qgssymbolv2.cpp
Expand Up @@ -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 );

Expand Down Expand Up @@ -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() ) )
{
Expand Down
1 change: 1 addition & 0 deletions src/gui/symbology-ng/qgs25drendererwidget.cpp
Expand Up @@ -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() );
Expand Down
2 changes: 1 addition & 1 deletion src/providers/virtual/qgsvirtuallayersqlitemodule.cpp
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/src/core/testqgsexpressioncontext.cpp
Expand Up @@ -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 );
}
Expand Down

4 comments on commit 8ad6ca0

@nyalldawson
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8ad6ca0 Jan 24, 2016

Choose a reason for hiding this comment

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

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
Copy link
Member

@m-kuhn m-kuhn commented on 8ad6ca0 Jan 24, 2016

Choose a reason for hiding this comment

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

Does that not work?

QgsExpressionContextScope* symbolScope( QgsSymbolV2* symbol, QgsExpressionContextScope* scope = nullptr /Transfer/ ) /TransferBack/;

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8ad6ca0 Jan 25, 2016

Choose a reason for hiding this comment

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

Updated comment based on 8ad6ca0#commitcomment-15646270

Please sign in to comment.