Warning: freeing local variable.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
- andrecaldas
- Posts: 301
- Joined: Fri Jan 27, 2023 8:45 pm
- Contact:
Warning: freeing local variable.
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.
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.
Re: Warning: freeing local variable.
Even the 3 last lines of the function don't make much sense.
@abdullah
@abdullah
Re: Warning: freeing local variable.
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.
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.
- adrianinsaval
- Veteran
- Posts: 5548
- Joined: Thu Apr 05, 2018 5:15 pm
Re: Warning: freeing local variable.
I get a 404 following this linkwmayer wrote: ↑Mon Mar 27, 2023 12:14 pm With git commit e4303310953de7 a virtual destructor has been added.
Re: Warning: freeing local variable.
Should be git commit 6b1e6a6, probably.adrianinsaval wrote: ↑Mon Mar 27, 2023 3:33 pmI get a 404 following this linkwmayer wrote: ↑Mon Mar 27, 2023 12:14 pm With git commit e4303310953de7 a virtual destructor has been added.
Cheers,
Markus
Markus
Re: Warning: freeing local variable.
Sorry, I copied the wrong part of the url.
- andrecaldas
- Posts: 301
- Joined: Fri Jan 27, 2023 8:45 pm
- Contact:
Re: Warning: freeing local variable.
@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
Re: Warning: freeing local variable.
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?
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?
- andrecaldas
- Posts: 301
- Joined: Fri Jan 27, 2023 8:45 pm
- Contact:
Re: Warning: freeing local variable.
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:
- Eliminate free(std::vector<Constraint*>) altogether by using std::vector<std::unique_ptr<Constraint>> instead of vectors of pointers.
- Do the same for std::vector<SubSystem*>.
What would you like?
Re: Warning: freeing local variable.
Sorry for the late reply.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:This way, no action is needed for destruction. And if anyone wants to reset the vector, s/he can simply call vector.clear().
- Eliminate free(std::vector<Constraint*>) altogether by using std::vector<std::unique_ptr<Constraint>> instead of vectors of pointers.
- Do the same for std::vector<SubSystem*>.
What would you like?
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?