Page MenuHomePhabricator

PlanarFigure interface provides access to non-consistent container
Closed, ResolvedPublic

Description

PlanarFigure currently has a method GetControlPoints() which returns an STL vector containing the control points of the figure. However, the container does not necessarily have the correct number of points set, for example in the case of PlanarPolygon, the number of elements can be one more than the actual number of control points, and the last element is undefined.

This can be a problem if this container is used for accessing control points directly, or for retrieving the number of control points.

Two possible solutions are:

  • Disallow direct access to this container (remove GetControlPoints() method); the user then needs to use the methods GetControlPoint( int ) and GetNumberOfControlPoints()
  • Make sure that the returned container is always consistent with the REAL number of control points

Event Timeline

I'm not able to reproduce the error. I tried it using the measurments tool and there it works correctly. Can you describe a precise scenaria in which it fails?

When creating the polygon the point that you are placing (but has not been set yet) is also counting to the number of points. Maybe this was a source of confusion?

(In reply to comment #1)

I'm not able to reproduce the error. I tried it using the measurments tool and
there it works correctly. Can you describe a precise scenaria in which it
fails?

The bug relates to a software design issue. As far as I know, there is currently no direct bug resulting from this.

A problem can occur if someone uses the methods to access the point container directly, and then tries to access points in it which are not defined or valid.

It may be reasonable to change the interface of PlanarFigure so that no direct access to the internal container is provided. However, the container access methods are used somewhere already, e.g. in the PlanarFigureMapper2D.

I still don't get it. When would a point be undefined or invalid?

To (re-)produce the problem I suggest to write a test that uses GetControlPoints with uninitialized values to produce some errors.

This bug has a high severity and was not fixed within the 2013-06 release. Setting target milestone to next release.

In the current code the mentioned function is no longer existant.
It seems the first suggest option has already been implemented.
Therefore closing this bug (as fixed).