Warning: freeing local variable.

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Warning: freeing local variable.

Post by andrecaldas »

I am getting a clang-analyzer warning here. I think the analyzer is correct on its complaints.

It's been more than 15 years since I did any serious C++ programing, but I guess this constrvec has no place in here. I guess it is supposed to be just delete *constr.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Warning: freeing local variable.

Post by openBrain »

Even the 3 last lines of the function don't make much sense.
@abdullah
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Warning: freeing local variable.

Post by wmayer »

Possibly the analyzer mixes up this function with C's free() function. This would indeed be wrong but in GCS.cpp there is an overloaded function free(std::vector<Constraint*>&).

I don't understand the purpose of this function because it does an explicit cast to the sub-type when deleting an element. Since the destructor of Constraint is declared as virtual there is no need for such a weird hack because the destructor of the sub class will be used automatically.

This function has already existed in 2011:
https://github.com/FreeCAD/FreeCAD/blob ... cs/GCS.cpp

Now when looking at the class declaration of Constraint at that time there was indeed no virtual destructor.
So, hard to say what the reason was doing it this way -- maybe some lacking knowledge of the original author.

With git commit e4303310953de7 a virtual destructor has been added.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Warning: freeing local variable.

Post by adrianinsaval »

wmayer wrote: Mon Mar 27, 2023 12:14 pm With git commit e4303310953de7 a virtual destructor has been added.
I get a 404 following this link
User avatar
mfro
Posts: 663
Joined: Sat Sep 23, 2017 8:15 am

Re: Warning: freeing local variable.

Post by mfro »

adrianinsaval wrote: Mon Mar 27, 2023 3:33 pm
wmayer wrote: Mon Mar 27, 2023 12:14 pm With git commit e4303310953de7 a virtual destructor has been added.
I get a 404 following this link
Should be git commit 6b1e6a6, probably.
Cheers,
Markus
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Warning: freeing local variable.

Post by wmayer »

Sorry, I copied the wrong part of the url.
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Re: Warning: freeing local variable.

Post by andrecaldas »

openBrain wrote: Mon Mar 27, 2023 10:51 am Even the 3 last lines of the function don't make much sense.
@abdullah
@openBrain and @wmayer...

Hello, you all!!! I am very very sorry... this is an old post. Probably I should have marked it as "solved" some how.
https://github.com/FreeCAD/FreeCAD/issues/8895
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Warning: freeing local variable.

Post by abdullah »

I am following it up (I have it unread along with quite some other notifications).

I think that originally the idea was some kind of optimisation. However, I do not think it makes sense anymore. What definitely makes no sense at all is to construct a vector to delete that pointer.

Another point in favour of change is that the code is misleading to read.

@andrecaldas

Are you willing to do a PR where you call delete+pointer instead of going all the indirect way into creating a vector to reuse the overloaded free function to finally delete the pointer?
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Re: Warning: freeing local variable.

Post by andrecaldas »

abdullah wrote: Mon Apr 03, 2023 3:36 pm @andrecaldas

Are you willing to do a PR where you call delete+pointer instead of going all the indirect way into creating a vector to reuse the overloaded free function to finally delete the pointer?
It seems to me that this free() is there because the author was afraid to delete the virtual class pointer. But I guess there is no need to do so, because the Constraint::~Constraint is properly declared virtual. Of course, that is if I am not missing anything. :-)

Since there is no special action needed to delete a Constraint, I can simply delete it directly.

But I can also:
  1. Eliminate free(std::vector<Constraint*>) altogether by using std::vector<std::unique_ptr<Constraint>> instead of vectors of pointers.
  2. Do the same for std::vector<SubSystem*>.
This way, no action is needed for destruction. And if anyone wants to reset the vector, s/he can simply call vector.clear().

What would you like?
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Warning: freeing local variable.

Post by abdullah »

andrecaldas wrote: Tue Apr 04, 2023 8:19 am It seems to me that this free() is there because the author was afraid to delete the virtual class pointer. But I guess there is no need to do so, because the Constraint::~Constraint is properly declared virtual. Of course, that is if I am not missing anything. :-)

Since there is no special action needed to delete a Constraint, I can simply delete it directly.

But I can also:
  1. Eliminate free(std::vector<Constraint*>) altogether by using std::vector<std::unique_ptr<Constraint>> instead of vectors of pointers.
  2. Do the same for std::vector<SubSystem*>.
This way, no action is needed for destruction. And if anyone wants to reset the vector, s/he can simply call vector.clear().

What would you like?
Sorry for the late reply.

I was asking only that, because (i) it is what originated the issue and it is clearly underperforming code (create an array to delete one pointer...), and (ii) not using the virtual destructor on all cases makes no sense to me at all. It is ugly codewise and I would not be there is an improvement of performance (decision on type comparison vs extra indirection of dispatch of inheritance), and (iii) it causes confusion to developers.

I have reflected on your proposal (1). What calls for the reflection is that at the solver performance is critical.

1) unique_ptr has a slight performance overhead over a regular C pointer (aka naked pointer) at construction/deletion. Without a fancy deleter it has the same footprint (memory).

2) the solver has very specific places for creation and deletion, and code does not rely on exceptions, which is one of the reasons to prefer smart pointers over regular ones (so that the code always passes through the deleting code and no memory leak happens). If the architecture would ensure no exception and always passing by the deletion function (let's assume it does), there cannot be more performing code using smart pointers.

3) then, one can call me names, and not nice ones, a fool would be the least, for the assumption that now and in the future, it can be assured that no developer would, even inadvertently create a path breaking this assumption. An example of a memory leak on the solver would certainly shake me out of this bubble thinking.

This is what is making this decision difficult for me. I am admittedly obsessed with solver performance, because on large recomputes of several sketches (for example due to changing a value of a master sketch in a Spreadsheet), there may be a lot of solver calls and thousands of constraints being created and destroyed.

But how much exactly this slight penalisation may be? I do not know. There is a good chance that it is negligible. Even there is an indication it will be, as most solver time goes into Eigen decompositions and iteration for which this does not have any effect at all.

If so, why so much scaremongering about performance, ...

Ok, let's try to bring some good logic here instead of relying on the horoscope.

If you feel like doing it, do (1). Then we will have to measure and compare, and decide whether to merge or throw away.

If you do not feel like, you may want to do the initial request.

We can talk about (2) after (1).

Does it sound reasonable?
Post Reply