PVS-Studio

[abdullah]

Happy to hear that the report was useful... Both thumbs up for fixing the issues this fast!

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]

Happy to hear that the report was useful... Both thumbs up for fixing the issues this fast!

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]

Happy to hear that the report was useful... Both thumbs up for fixing the issues this fast!

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.

[fosselius]

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]

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]

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]

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]

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]

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!).

[saso]

New report has been created from build 0.19.16291...