PVS-Studio

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

Re: PVS-Studio

Postby abdullah » Wed Mar 13, 2019 4:30 pm

wmayer wrote:
Wed Mar 13, 2019 4:26 pm
abdullah wrote:
Wed Mar 13, 2019 3:19 pm
saso wrote:
Mon Mar 11, 2019 10:10 pm
Happy to hear that the report was useful... Both thumbs up for fixing the issues this fast! :shock:
I have been looking to what Werner has pushed and I was very impressed with the detection capabilities of this tool.
At the moment there are left 15 issues I will have a look at. If you have some time then I will appreciate it if you can check the issues in CommandConstraints.cpp:4632, ..., with the description: Possible incorrect order of arguments passed to 'makeTangentToArcOfHyperbolaviaNewPoint' function: 'geom2' and 'geom1'.
I do it now.
abdullah
Posts: 2822
Joined: Sun May 04, 2014 3:16 pm

Re: PVS-Studio

Postby abdullah » Wed Mar 13, 2019 4:39 pm

wmayer wrote:
Wed Mar 13, 2019 4:26 pm
abdullah wrote:
Wed Mar 13, 2019 3:19 pm
saso wrote:
Mon Mar 11, 2019 10:10 pm
Happy to hear that the report was useful... Both thumbs up for fixing the issues this fast! :shock:
I have been looking to what Werner has pushed and I was very impressed with the detection capabilities of this tool.
At the moment there are left 15 issues I will have a look at. If you have some time then I will appreciate it if you can check the issues in CommandConstraints.cpp:4632, ..., with the description: Possible incorrect order of arguments passed to 'makeTangentToArcOfHyperbolaviaNewPoint' function: 'geom2' and 'geom1'.
The code is correct. This function expects as second parameter a Part::GeomArcOfHyperbola. I think it is detecting that a variable named geom2 is being passed to a parameter named geom1 and viceversa.

If there is a need to fix this, so that the warning does not appear in subsequent runs, we may rename the parameters of makeTangentToArcOfHyperbolaviaNewPoint, for example what today is "geom1" could be renamed to "hyperbola".
abdullah
Posts: 2822
Joined: Sun May 04, 2014 3:16 pm

Re: PVS-Studio

Postby abdullah » Wed Mar 13, 2019 4:40 pm

wmayer wrote:
Wed Mar 13, 2019 4:26 pm
abdullah wrote:
Wed Mar 13, 2019 3:19 pm
saso wrote:
Mon Mar 11, 2019 10:10 pm
Happy to hear that the report was useful... Both thumbs up for fixing the issues this fast! :shock:
I have been looking to what Werner has pushed and I was very impressed with the detection capabilities of this tool.
At the moment there are left 15 issues I will have a look at. If you have some time then I will appreciate it if you can check the issues in CommandConstraints.cpp:4632, ..., with the description: Possible incorrect order of arguments passed to 'makeTangentToArcOfHyperbolaviaNewPoint' function: 'geom2' and 'geom1'.
If you think I am capable of supporting you any other one, let me know. :)
User avatar
fosselius
Posts: 288
Joined: Sat Apr 23, 2016 10:03 am

Re: PVS-Studio

Postby fosselius » Wed Mar 13, 2019 4:57 pm

abdullah wrote:
Wed Mar 13, 2019 4:39 pm
The code is correct. This function expects as second parameter a Part::GeomArcOfHyperbola. I think it is detecting that a variable named geom2 is being passed to a parameter named geom1 and viceversa.
That sounds like an code quality issue, can probably be a bit confusing for current/future developers ^_^

https://github.com/FreeCAD/FreeCAD/blob ... raints.cpp

Code: Select all


makeTangentToEllipseviaNewPoint(Obj,geom1,geom2,GeoId1,GeoId2)
makeTangentToArcOfHyperbolaviaNewPoint(Obj,geom2,geom1,GeoId2,GeoId1)
makeTangentToArcOfParabolaviaNewPoint(Obj,geom2,geom1,GeoId2,GeoId1)

Code: Select all

// Makes a simple tangency constraint using extra point + tangent via point
/// geom1 => an arc of hyperbola
/// geom2 => any of an arc of hyperbola, an arc of ellipse, a circle, or an arc (of circle)
/// NOTE: A command must be opened before calling this function, which this function
/// commits or aborts as appropriate. The reason is for compatibility reasons with
/// other code e.g. "Autoconstraints" in DrawSketchHandler.cpp
void SketcherGui::makeTangentToArcOfHyperbolaviaNewPoint(Sketcher::SketchObject* Obj,
                                                       const Part::Geometry *geom1,
                                                       const Part::Geometry *geom2,
                                                       int geoId1,
                                                       int geoId2
)
{
    const Part::GeomArcOfHyperbola *aoh = static_cast<const Part::GeomArcOfHyperbola *>(geom1);

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

Re: PVS-Studio

Postby abdullah » Wed Mar 13, 2019 6:59 pm

fosselius wrote:
Wed Mar 13, 2019 4:57 pm
abdullah wrote:
Wed Mar 13, 2019 4:39 pm
The code is correct. This function expects as second parameter a Part::GeomArcOfHyperbola. I think it is detecting that a variable named geom2 is being passed to a parameter named geom1 and viceversa.
That sounds like an code quality issue, can probably be a bit confusing for current/future developers ^_^
There some positive aspects to it:
1) the function is well documented. It indicates that the second parameter, which the function calls geom1 is the hyperbola.
2) the function is not responsible for the name the client code gives to the variables passed to it as parameters.
3) the client code is not abusing a terminology (see below), in that what the client code calls 'geom2' can be a lot of different geometries.

However I do agree it is indeed an unfortunate coincidence.

To improve this, what I proposed above, was to change the name of the parameter of the function 'geom1' to 'hyperbola'. But PVS-studio may still complaint nevertheless because of 'geom1' in the client code. I am not sure what PVS is detecting exactly.

Probably, a better option would be to change the type of the first parameter from 'const Part::Geometry *' to 'const Part::GeomArcOfHyperbola *'. This would leverage the compiler to note possible mistakes by the user at compilation time instead of "trusting" the client code will do appropriate type checking. This is actually more dangerous for future developers in that the first thing the function does in an static_cast to Part::GeomArcOfHyperbola without any additional checking.

In fact, we could actually do both, change the type and the name.

This code was originally repeated in several places and then it was refactored, probably it was me (mea culpa), and a better job could have been done.

I was just answering Werner's question, but I can prepare a PR for v0.19 with this if he agrees to the solution. I could also apply the same pattern to the other two or so similar functions that were refactored for ellipses and parabolas.

Code: Select all

               if( geom2->getTypeId() == Part::GeomEllipse::getClassTypeId() ||
                    geom2->getTypeId() == Part::GeomArcOfEllipse::getClassTypeId() ||
                    geom2->getTypeId() == Part::GeomCircle::getClassTypeId() ||
                    geom2->getTypeId() == Part::GeomArcOfCircle::getClassTypeId() ) {

                    Gui::Command::openCommand("add tangent constraint point");
                    makeTangentToEllipseviaNewPoint(Obj,geom1,geom2,GeoId1,GeoId2);
                    getSelection().clearSelection();
                    return;
                }
                else if( geom2->getTypeId() == Part::GeomArcOfHyperbola::getClassTypeId() ) {
                    Gui::Command::openCommand("add tangent constraint point");
                    makeTangentToArcOfHyperbolaviaNewPoint(Obj,geom2,geom1,GeoId2,GeoId1);
                    getSelection().clearSelection();
                    return;
                }
                else if( geom2->getTypeId() == Part::GeomArcOfParabola::getClassTypeId() ) {
                    Gui::Command::openCommand("add tangent constraint point");
                    makeTangentToArcOfParabolaviaNewPoint(Obj,geom2,geom1,GeoId2,GeoId1);
                    getSelection().clearSelection();
                    return;
                }
 
wmayer
Site Admin
Posts: 13815
Joined: Thu Feb 19, 2009 10:32 am

Re: PVS-Studio

Postby wmayer » Thu Mar 14, 2019 8:49 am

abdullah wrote:
Wed Mar 13, 2019 4:39 pm
The code is correct. This function expects as second parameter a Part::GeomArcOfHyperbola. I think it is detecting that a variable named geom2 is being passed to a parameter named geom1 and viceversa.

If there is a need to fix this, so that the warning does not appear in subsequent runs, we may rename the parameters of makeTangentToArcOfHyperbolaviaNewPoint, for example what today is "geom1" could be renamed to "hyperbola".
Thanks for taking a look. If you like you can adjust the code but there is no need for it, IMO.
abdullah
Posts: 2822
Joined: Sun May 04, 2014 3:16 pm

Re: PVS-Studio

Postby abdullah » Thu Mar 14, 2019 6:29 pm

wmayer wrote:
Thu Mar 14, 2019 8:49 am
abdullah wrote:
Wed Mar 13, 2019 4:39 pm
The code is correct. This function expects as second parameter a Part::GeomArcOfHyperbola. I think it is detecting that a variable named geom2 is being passed to a parameter named geom1 and viceversa.

If there is a need to fix this, so that the warning does not appear in subsequent runs, we may rename the parameters of makeTangentToArcOfHyperbolaviaNewPoint, for example what today is "geom1" could be renamed to "hyperbola".
Thanks for taking a look. If you like you can adjust the code but there is no need for it, IMO.
Thanks. I have prepared a PR;
https://github.com/FreeCAD/FreeCAD/pull/2017
wmayer
Site Admin
Posts: 13815
Joined: Thu Feb 19, 2009 10:32 am

Re: PVS-Studio

Postby wmayer » Sat Mar 16, 2019 11:36 am

wandererfan wrote: ping
There is another issue in TechDraw: The class QGIVertex has the member projIndex and the method getProjIndex. The sub-class QGICMark redefines the same member variable and function which seems to be a bit odd. Is this on purpose?
wandererfan
Posts: 2389
Joined: Tue Nov 06, 2012 5:42 pm

Re: PVS-Studio

Postby wandererfan » Sun Mar 17, 2019 1:11 pm

wmayer wrote:
Sat Mar 16, 2019 11:36 am
There is another issue in TechDraw: The class QGIVertex has the member projIndex and the method getProjIndex. The sub-class QGICMark redefines the same member variable and function which seems to be a bit odd. Is this on purpose?
Good catch. It's an error. PR submitted (and merged!).