Need some help from a C++ expert (or a shrink)

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
abdullah
Posts: 4002
Joined: Sun May 04, 2014 3:16 pm

Need some help from a C++ expert (or a shrink)

Postby abdullah » Sun Sep 06, 2015 5:02 pm

Hi folks,

I am running into an issue I do not understand.

In this branch (_external_geometry_overconstrained_sketch_2), I have this commit:
https://github.com/abdullahtahiriyo/Fre ... 17500a544d

That introduces two functions:

Code: Select all

    // retrieves an array of maps, each map containing the points that are coincidence by virtue of 
    // any number of direct or indirect coincidence constraints
    const std::vector< std::map<int, Sketcher::PointPos> > & getCoincidenceGroups();
    // returns if the given geoId is fixed (coincident) with external geometry on any of the possible relevant points
    void isCoincidentWithExternalGeometry(int GeoId, bool &start_external, bool &mid_external, bool &end_external);
This code will make FC crash whenever isCoincidentWithExternalGeometry is called (see example of how to make it crash below, just if needed).

In this branch (_external_geometry_overconstrained_sketch_3), I have this commit:
https://github.com/abdullahtahiriyo/Fre ... f532c0a03d

which is just one commit over the contents of branch2.

The difference between branch3 and branch2 is:

Code: Select all

diff --git a/src/Mod/Sketcher/App/SketchObject.cpp b/src/Mod/Sketcher/App/SketchObject.cpp
index cdb46e9..12f28f3 100644
--- a/src/Mod/Sketcher/App/SketchObject.cpp
+++ b/src/Mod/Sketcher/App/SketchObject.cpp
@@ -3143,7 +3143,54 @@ void SketchObject::isCoincidentWithExternalGeometry(int GeoId, bool &start_exter
     
     const std::vector< Sketcher::Constraint * > &vals = Constraints.getValues();
     
-    const std::vector< std::map<int, Sketcher::PointPos> > &coincidenttree = getCoincidenceGroups();
+    std::vector< std::map<int, Sketcher::PointPos> > coincidenttree;^M
+    // push the constraints^M
+    for (std::vector< Sketcher::Constraint * >::const_iterator it= vals.begin();it != vals.end(); ++it) {^M
+        if( (*it)->Type == Sketcher::Coincident ) {^M
+            int firstpresentin=-1;^M
+            int secondpresentin=-1;^M
+            ^M
+            int i=0;^M
+            ^M
+            for(std::vector< std::map<int, Sketcher::PointPos> >::const_iterator iti = coincidenttree.begin(); iti != coincidenttree.end(); ++iti,i++) {^M
+                // First^M
+                std::map<int, Sketcher::PointPos>::const_iterator filiterator;^M
+                filiterator = (*iti).find((*it)->First);^M
+                if( filiterator != (*iti).end()) {^M
+                    if((*it)->FirstPos == (*filiterator).second)^M
+                        firstpresentin = i;^M
+                }^M
+                // Second^M
+                filiterator = (*iti).find((*it)->Second);^M
+                if( filiterator != (*iti).end()) {^M
+                    if((*it)->SecondPos == (*filiterator).second)                                ^M
+                        secondpresentin = i;^M
+                }                             ^M
+            }^M will make FC crash whenever isCoincidentWithExternalGeometry is called
+            ^M
+            if ( firstpresentin!=-1 && secondpresentin!=-1) {^M
+                // we have to merge those sets into one^M
+                coincidenttree[firstpresentin].insert(coincidenttree[secondpresentin].begin(), coincidenttree[secondpresentin].end());^M
+                coincidenttree.erase(coincidenttree.begin()+secondpresentin);^M
+            }^M
+            else if ( firstpresentin==-1 && secondpresentin==-1 ) {^M
+                // we do not have any of the values, so create a setCursor^M
+                std::map<int, Sketcher::PointPos> tmp;^M
+                tmp.insert(std::pair<int, Sketcher::PointPos>((*it)->First,(*it)->FirstPos));^M
+                tmp.insert(std::pair<int, Sketcher::PointPos>((*it)->Second,(*it)->SecondPos));^M
+                coincidenttree.push_back(tmp);^M
+            }^M
+            else if ( firstpresentin != -1 ) {^M
+                // add to existing group^M
+                coincidenttree[firstpresentin].insert(std::pair<int, Sketcher::PointPos>((*it)->Second,(*it)->SecondPos));^M
+            }^M
+            else { // secondpresentin != -1^M
+                // add to existing group^M
+                coincidenttree[secondpresentin].insert(std::pair<int, Sketcher::PointPos>((*it)->First,(*it)->FirstPos));^M
+            }^M
+            ^M
+        }^M
+    }^M
     
     for(std::vector< std::map<int, Sketcher::PointPos> >::const_iterator it = coincidenttree.begin(); it != coincidenttree.end(); ++it) {
So basically, the difference between branches 2 and 3 is that 3 removes the call to getCoincidenceGroups(); and includes its code directly inside isCoincidentWithExternalGeometry instead.

Branch 3 works perfectly well and does not crash.

I think it may be related to the fact that I am returning by reference a local variable (yes, for a normal variable there is a memory leak because the variable is destroyed upon leaving the function, so the reference points to an address with invalid/destroyed data), but according to my knowledge of c++ (which may be wrong or outdated), it is perfectly ok to do that if the reference is const. In that specific case, the call to destroy the object is deferred to the point where the reference is destroyed.

I can not extract enough information from debugging with kdevelop branch 2 as to why it crashes. It does it, after several iterations in the loop in line (inside the STL find routine, at the compare subroutine):

Code: Select all

        geoId1iterator = (*it).find(GeoId);
I post here because I am running crazy as to why, and I would like to have both functions separate because I plan to use const std::vector< std::map<int, Sketcher::PointPos> > & getCoincidenceGroups() in other places unrelated to external geometry.

How to reproduce the crash:
1. Download the sketch of this post:
viewtopic.php?f=10&t=12254#p98522
2. Open Sketch6
3. Select create polyline, click coincident with the point on the vertical axis (the one that is not in the origin), next click on the point of the origin.
4. You should have crashed it by now.

Thanks in advance,
abdullah
User avatar
DeepSOIC
Posts: 7833
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Need some help from a C++ expert (or a shrink)

Postby DeepSOIC » Sun Sep 06, 2015 6:35 pm

Hi!
abdullah wrote:I think it may be related to the fact that I am returning by reference a local variable (yes, for a normal variable there is a memory leak because the variable is destroyed upon leaving the function, so the reference points to an address with invalid/destroyed data), but according to my knowledge of c++ (which may be wrong or outdated), it is perfectly ok to do that if the reference is const. In that specific case, the call to destroy the object is deferred to the point where the reference is destroyed.
Hmm. No, I'm not close to being an expert in c++. But to be honest, this is the first time I hear about it, and honestly, it doesn't make any sense to me. I can suggest you make an experiment: create a class with a non-empty destructor, return a const reference to it, and step through the code to see if the destructor is called.
Fat-Zer
Posts: 171
Joined: Thu Oct 30, 2014 10:38 pm

Re: Need some help from a C++ expert (or a shrink)

Postby Fat-Zer » Sun Sep 06, 2015 7:04 pm

abdullah wrote:I think it may be related to the fact that I am returning by reference a local variable (yes, for a normal variable there is a memory leak because the variable is destroyed upon leaving the function, so the reference points to an address with invalid/destroyed data), but according to my knowledge of c++ (which may be wrong or outdated), it is perfectly ok to do that if the reference is const. In that specific case, the call to destroy the object is deferred to the point where the reference is destroyed.
yes, it's your mistake, you can't _return_ a temporary object by reference even by a constant reference, it seems you confused it with that you can _pass_ a temporary object to a function that takes a constant reference (but not a normal reference).

Note since C++11 a vector returned by value won't call a deep data copy... and as far as freecad usees C++03, gcc will likely optimize it out anyway...

PS: Compiler should have warned you about that...
Last edited by Fat-Zer on Sun Sep 06, 2015 7:26 pm, edited 2 times in total.
eivindkvedalen
Posts: 602
Joined: Tue Jan 29, 2013 10:35 pm

Re: Need some help from a C++ expert (or a shrink)

Postby eivindkvedalen » Sun Sep 06, 2015 7:07 pm

abdullah wrote:Hi folks,

I am running into an issue I do not understand.

In this branch (_external_geometry_overconstrained_sketch_2), I have this commit:
https://github.com/abdullahtahiriyo/Fre ... 17500a544d

....

I post here because I am running crazy as to why, and I would like to have both functions separate because I plan to use const std::vector< std::map<int, Sketcher::PointPos> > & getCoincidenceGroups() in other places unrelated to external geometry.

How to reproduce the crash:
1. Download the sketch of this post:
viewtopic.php?f=10&t=12254#p98522
2. Open Sketch6
3. Select create polyline, click coincident with the point on the vertical axis (the one that is not in the origin), next click on the point of the origin.
4. You should have crashed it by now.

Thanks in advance,
abdullah
I'm not able to recreate the crash, but returning a reference to local variable is going to get you in trouble: the vector coincidenttree is being destroyed when it goes out of scope, and you then return a reference to something that isn't there anymore. Remove the & in your method return type, and you should be fine.

Eivind
abdullah
Posts: 4002
Joined: Sun May 04, 2014 3:16 pm

Re: Need some help from a C++ expert (or a shrink)

Postby abdullah » Sun Sep 06, 2015 7:52 pm

Thanks guys for your replies.

I somehow read this a while ago and never tried it. I have tried to find references and I found this:
http://herbsutter.com/2008/01/01/gotw-8 ... ant-const/
http://stackoverflow.com/questions/4643 ... l-variable

The last reply here shreds some light into why several people are asking this around in programming forums:
http://www.thecodingforums.com/threads/ ... ry.743990/

In my case I will go with Eric's suggestion and rely on compiler optimizations in cases it is big.
eivindkvedalen
Posts: 602
Joined: Tue Jan 29, 2013 10:35 pm

Re: Need some help from a C++ expert (or a shrink)

Postby eivindkvedalen » Sun Sep 06, 2015 7:56 pm

abdullah wrote:Thanks guys for your replies.

I somehow read this a while ago and never tried it. I have tried to find references and I found this:
http://herbsutter.com/2008/01/01/gotw-8 ... ant-const/
http://stackoverflow.com/questions/4643 ... l-variable

The last reply here shreds some light into why several people are asking this around in programming forums:
http://www.thecodingforums.com/threads/ ... ry.743990/

In my case I will go with Eric's suggestion and rely on compiler optimizations in cases it is big.
If you want to be sure not to copy the vector on return, you can declare it as a an extra non-const reference input parameter.

Eivind
Fat-Zer
Posts: 171
Joined: Thu Oct 30, 2014 10:38 pm

Re: Need some help from a C++ expert (or a shrink)

Postby Fat-Zer » Tue Sep 08, 2015 8:15 pm

eivindkvedalen wrote:If you want to be sure not to copy the vector on return, you can declare it as a an extra non-const reference input parameter.
Or use std::vector::swap instead of assignment...