kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

D'oh, and I though I was being so clever with git send-email... Here
it is all in one piece.

Sorry!

John


On Wed, Jan 23, 2019 at 2:46 PM Seth Hillbrand <seth@xxxxxxxxxxxxx> wrote:
>
> Am 2019-01-23 08:46, schrieb John Beard:
> > In the past, the ZONE_CONTAINER::GetPosition() member
> > returned a reference to a wxPoint, in accordance with the
> > BOARD_ITEM interface. This meant that ZONE_CONTAINER had
> > to reinterpret_cast a VECTOR2I (its internal position data) to wxPoint,
> > which was possible only due to fortunate memory layout.
> >
> > This interface was changed to return wxPoints by value in
> > commit a4528988ca, and the normal (wxPoint) cast was used instead.
> >
> > Thus, we can now also remove the dire warnings and static_asserts
> > used to ensure the now-unused old method was correct.
> > ---
> >  pcbnew/class_zone.h | 46 ---------------------------------------------
> >  1 file changed, 46 deletions(-)
>
> Makes sense.  Would you be able to send this one with git format-patch?

From 29845d0304709283c8b48a36cc507151a28d8b32 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Wed, 23 Jan 2019 12:46:20 +0000
Subject: [PATCH] Zones: wxPoint/VECTOR2I warnings no longer needed

In the past, the ZONE_CONTAINER::GetPosition() member
returned a reference to a wxPoint, in accordance with the
BOARD_ITEM interface. This meant that ZONE_CONTAINER had
to reinterpret_cast a VECTOR2I (its internal position data) to wxPoint,
which was possible only due to fortunate memory layout.

This interface was changed to return wxPoints by value in
commit a4528988ca, and the normal (wxPoint) cast was used instead.

Thus, we can now also remove the dire warnings and static_asserts
used to ensure the now-unused old method was correct.
---
 pcbnew/class_zone.h | 46 ---------------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/pcbnew/class_zone.h b/pcbnew/class_zone.h
index b59f8a710..ebcfca1ea 100644
--- a/pcbnew/class_zone.h
+++ b/pcbnew/class_zone.h
@@ -79,15 +79,6 @@ public:
     }
 
     /**
-     * Function GetPosition
-     *
-     * Returns a reference to the first corner of the polygon set.
-     *
-     * \warning The implementation of this function relies on the fact that wxPoint and VECTOR2I
-     * have the same layout. If you intend to use the returned reference directly, please note
-     * that you are _only_ allowed to use members x and y. Any use on anything that is not one of
-     * these members will have undefined behaviour.
-     *
      * @return a wxPoint, position of the first point of the outline
      */
     const wxPoint GetPosition() const override;
@@ -770,43 +761,6 @@ private:
     std::vector<int>      m_insulatedIslands;
 
     bool                  m_hv45;           // constrain edges to horizontal, vertical or 45º
-
-    /**
-     * Union to handle conversion between references to wxPoint and to VECTOR2I.
-     *
-     * The function GetPosition(), that returns a reference to a wxPoint, needs some existing
-     * wxPoint object that it can point to. The header of this function cannot be changed, as it
-     * overrides the function from the base class BOARD_ITEM. This made sense when ZONE_CONTAINER
-     * was implemented using the legacy CPolyLine class, that worked with wxPoints. However,
-     * m_Poly is now a SHAPE_POLY_SET, whose corners are objects of type VECTOR2I, not wxPoint.
-     * Thus, we cannot directly reference the first corner of m_Poly, so a modified version of it
-     * that can be read as a wxPoint needs to be handled.
-     * Taking advantage of the fact that both wxPoint and VECTOR2I have the same memory layout
-     * (two integers: x, y), this union let us convert a reference to a VECTOR2I into a reference
-     * to a wxPoint.
-     *
-     * The idea is the following: in GetPosition(), m_Poly->GetCornerPosition( 0 ) returns a
-     * reference to the first corner of the polygon set. If we retrieve its memory direction, we
-     * can tell the compiler to cast that pointer to a WX_VECTOR_CONVERTER pointer. We can finally
-     * shape that memory layout as a wxPoint picking the wx member of the union.
-     *
-     * Although this solution is somewhat unstable, as it relies on the fact that the memory
-     * layout is exactly the same, it is the best attempt to keep backwards compatibility while
-     * using the new SHAPE_POLY_SET.
-     */
-    typedef union {
-        wxPoint wx;
-        VECTOR2I vector;
-    } WX_VECTOR_CONVERTER;
-
-    // Sanity check: assure that the conversion VECTOR2I->wxPoint using the previous union is
-    // correct, making sure that the access for x and y attributes is still safe.
-    static_assert(offsetof(wxPoint,x) == offsetof(VECTOR2I,x),
-                  "wxPoint::x and VECTOR2I::x have different offsets");
-
-    static_assert(offsetof(wxPoint,y) == offsetof(VECTOR2I,y),
-                  "wxPoint::y and VECTOR2I::y have different offsets");
-
 };
 
 
-- 
2.20.1