kindred/create
1
1
Fork 0
Code Issues 37 Pull Requests Actions Packages Projects 9 Releases 2 Wiki Activity

Sketcher: Coverity fix in Sketch::analyseBlockedGeometry

Browse Source 
========================================================

Users chennes and hyarion made me aware of this covereity issue:

Fixes Coverity: geoit can be end() when dereferenced
https://github.com/FreeCAD/FreeCAD/pull/4429/files#

When analysing the block where the dereferrencing appears, it
appears that it is a left-over that no longer makes sense:
- The algorithm classifies block constraints into those that are
not affected by any other driving constraint and those that are
affected by other driving constraints.
- The offending block deals with internal aligned geometry, thus
per definition has a driving internal alignment constraint, for which
the previous block already set the need of post-analysis.
- No matter what, the geometries, the complex one and the internal one
will have at least the driving internal alignment constraint, so they
cannot become "not affected by any other driving constraint".
- If the geometry had a block constraint on it, it was already added for
post-analysis in the previous block. If it did not have one block constraint,
the fact that it is internal aligned geometry is an irrelevant consideration.

Probably there was a point during development when this made sense, but with
the current post-analysis, it does not appear to make sense anymore. So the
block was removed.

This commit adds a unit test for blocked geometry (new block constraint).
This commit is contained in:
Abdullah Tahiri
2021-02-12 07:25:20 +01:00
parent 16b8e02969
commit 30a93b648b
2 changed files with 199 additions and 193 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
src/Mod/Sketcher/App/Sketch.cpp
View File

@@ -138,7 +138,9 @@ bool Sketch::analyseBlockedGeometry( const std::vector<Part::Geometry *> &intern
std::vector<bool> &onlyblockedGeometry,
std::vector<int> &blockedGeoIds) const
{
bool isSomethingBlocked = false;
// To understand this function read the documentation in Sketch.h
// It is important that "onlyblockedGeometry" ONLY identifies blocked geometry
// that is not affected by any other driving constraint
bool doesBlockAffectOtherConstraints = false;
int geoindex = 0;
@@ -152,7 +154,7 @@ bool Sketch::analyseBlockedGeometry( const std::vector<Part::Geometry *> &intern
// is block driving
if( c->Type == Sketcher::Block && c->isDriving && c->First == geoindex)
blockisDriving = true;
// We have another driving constraint (which may be InternalAlignment)
if( c->Type != Sketcher::Block && c->isDriving &&
(c->First == geoindex || c->Second == geoindex || c->Third == geoindex) )
blockOnly = false;
@@ -161,12 +163,10 @@ bool Sketch::analyseBlockedGeometry( const std::vector<Part::Geometry *> &intern
if(blockisDriving) {
if(blockOnly) {
onlyblockedGeometry[geoindex] = true; // we pre-fix this geometry
isSomethingBlocked = true;
}
else {
// we will have to pos-analyse the first diagnose result for these geometries
// in order to avoid redundant constraints
isSomethingBlocked = true;
doesBlockAffectOtherConstraints = true;
blockedGeoIds.push_back(geoindex);
}
@@ -176,39 +176,6 @@ bool Sketch::analyseBlockedGeometry( const std::vector<Part::Geometry *> &intern
geoindex++;
}
if(isSomethingBlocked) {
// look for internal geometry linked IAs
for(auto c : constraintList) {
if(c->Type == InternalAlignment) {
auto geoit = std::find(blockedGeoIds.begin(),blockedGeoIds.end(),c->Second);
if(geoit != blockedGeoIds.end() || onlyblockedGeometry[c->Second]) { // internal alignment geometry found, add to list
// check if pre-fix or post-analyses
bool blockAffectedOnly = true;
for(auto ic : constraintList) {
// there is another driving constraint
if( ic->Type != Sketcher::Block && ic->isDriving &&
(ic->First == c->First || ic->Second == c->First || ic->Third == c->First))
blockAffectedOnly = false;
}
if(blockAffectedOnly) {
onlyblockedGeometry[c->Second] = true; // we pre-fix this geometry
}
else {
// we will have to post-analyse the first diagnose result for these geometries
// in order to avoid redundant constraints
doesBlockAffectOtherConstraints = true;
blockedGeoIds.push_back(*geoit);
}
}
}
}
}
return doesBlockAffectOtherConstraints;
}
@@ -247,19 +214,23 @@ int Sketch::setUpSketch(const std::vector<Part::Geometry *> &GeoList,
Base::Console().Log("\nOnlyBlocked GeoIds:");
size_t i = 0;
bool found = false;
for(; i < onlyBlockedGeometry.size(); i++) {
if(onlyBlockedGeometry[i])
if(onlyBlockedGeometry[i]) {
Base::Console().Log("\n GeoId=%d", i);
found = true;
}
}
if( i == 0)
if(found)
Base::Console().Log("\n None");
Base::Console().Log("\nNotOnlyBlocked GeoIds:");
i = 0;
for(; i < blockedGeoIds.size(); i++)
Base::Console().Log("\n GeoId=%d", blockedGeoIds[i]);
if( i == 0)
if(i == 0)
Base::Console().Log("\n None");
Base::Console().Log("\n");
#endif //DEBUG_BLOCK_CONSTRAINT
addGeometry(intGeoList,onlyBlockedGeometry);