Re: [Kicad-developers] [Patch] Fix initialization order for COLOR4D statics
Thanks Ian. This looks good. Patch merged. -Seth On 2019-08-05 10:39, Ian McInerney wrote: The updated patch using constexpr (which now only affects the color4d files) is attached. Simon, thanks for the suggestion to use constexpr. It ended up working by just declaring the variables constexpr and also making one of the constructors constexpr. Definitely much simpler. -Ian On Mon, Aug 5, 2019 at 2:10 PM Wayne Stambaugh wrote: On 8/5/19 5:03 AM, Simon Richter wrote: Hi, On Mon, Aug 05, 2019 at 10:58:44AM +0200, Ian McInerney wrote: I tracked it down to the fact that COLOR4D has some static colors that were being used to initialize some other static variables in pcbnew's layer widgets. I have moved all the static colors to an initialize on first use paradigm (so now we just call them like a function, e.g. COLOR4D::UNSPECIFIED() ) to fix it. Can that be solved by constexpr? Simon I would prefer the constexpr solution if it resolves the issue. It certainly would be a much simpler patch. Wayne ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] [Patch] Fix initialization order for COLOR4D statics
The updated patch using constexpr (which now only affects the color4d
files) is attached.
Simon, thanks for the suggestion to use constexpr. It ended up working by
just declaring the variables constexpr and also making one of the
constructors constexpr. Definitely much simpler.
-Ian
On Mon, Aug 5, 2019 at 2:10 PM Wayne Stambaugh wrote:
> On 8/5/19 5:03 AM, Simon Richter wrote:
> > Hi,
> >
> > On Mon, Aug 05, 2019 at 10:58:44AM +0200, Ian McInerney wrote:
> >
> >> I tracked it down to the fact that COLOR4D has some static colors that
> were
> >> being used to initialize some other static variables in pcbnew's layer
> >> widgets. I have moved all the static colors to an initialize on first
> use
> >> paradigm (so now we just call them like a function, e.g.
> >> COLOR4D::UNSPECIFIED() ) to fix it.
> >
> > Can that be solved by constexpr?
> >
> >Simon
> >
>
> I would prefer the constexpr solution if it resolves the issue. It
> certainly would be a much simpler patch.
>
> Wayne
>
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help : https://help.launchpad.net/ListHelp
>
From 06c283922ae6c01be0de45c74c25185c6884955a Mon Sep 17 00:00:00 2001
From: Ian McInerney
Date: Mon, 5 Aug 2019 15:54:13 +0200
Subject: [PATCH] Fix initialization of COLOR4D statics
Just declaring as static const would give an initialization order
fiasco since they were being used to initialize other statics.
---
common/gal/color4d.cpp | 6 +++---
include/gal/color4d.h | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/common/gal/color4d.cpp b/common/gal/color4d.cpp
index bc71f45bc..eadc3c220 100644
--- a/common/gal/color4d.cpp
+++ b/common/gal/color4d.cpp
@@ -362,6 +362,6 @@ COLOR4D& COLOR4D::Saturate( double aFactor )
return *this;
}
-const COLOR4D COLOR4D::UNSPECIFIED( 0, 0, 0, 0 );
-const COLOR4D COLOR4D::WHITE( 1, 1, 1, 1 );
-const COLOR4D COLOR4D::BLACK( 0, 0, 0, 1 );
+constexpr COLOR4D COLOR4D::UNSPECIFIED( 0, 0, 0, 0 );
+constexpr COLOR4D COLOR4D::WHITE( 1, 1, 1, 1 );
+constexpr COLOR4D COLOR4D::BLACK( 0, 0, 0, 1 );
diff --git a/include/gal/color4d.h b/include/gal/color4d.h
index 1b3932623..acd48205e 100644
--- a/include/gal/color4d.h
+++ b/include/gal/color4d.h
@@ -2,7 +2,7 @@
* This program source code file is part of KICAD, a free EDA CAD application.
*
* Copyright (C) 2012 Torsten Hueter, torstenhtr gmx.de
- * Copyright (C) 2017 Kicad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 2017-2019 Kicad Developers, see AUTHORS.txt for contributors.
*
* Color class
*
@@ -53,7 +53,7 @@ public:
* @param aBlue is the blue component [0.0 .. 1.0].
* @param aAlpha is the alpha value [0.0 .. 1.0].
*/
-COLOR4D( double aRed, double aGreen, double aBlue, double aAlpha ) :
+constexpr COLOR4D( double aRed, double aGreen, double aBlue, double aAlpha ) :
r( aRed ), g( aGreen ), b( aBlue ), a( aAlpha )
{
assert( r >= 0.0 && r <= 1.0 );
--
2.21.0
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] [Patch] Fix initialization order for COLOR4D statics
On 8/5/19 5:03 AM, Simon Richter wrote: > Hi, > > On Mon, Aug 05, 2019 at 10:58:44AM +0200, Ian McInerney wrote: > >> I tracked it down to the fact that COLOR4D has some static colors that were >> being used to initialize some other static variables in pcbnew's layer >> widgets. I have moved all the static colors to an initialize on first use >> paradigm (so now we just call them like a function, e.g. >> COLOR4D::UNSPECIFIED() ) to fix it. > > Can that be solved by constexpr? > >Simon > I would prefer the constexpr solution if it resolves the issue. It certainly would be a much simpler patch. Wayne ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
Re: [Kicad-developers] [Patch] Fix initialization order for COLOR4D statics
Forgot that I split the updating the widget into a separate commit. Here is
an updated patch that just fixes the initialization (I updated the message).
-Ian
On Mon, Aug 5, 2019 at 10:58 AM Ian McInerney
wrote:
> I decided to instrument my debug build with the address sanitizer and see
> what happened, and it has pointed out an initialization order fiasco in the
> loading of pcbnew.
>
> I tracked it down to the fact that COLOR4D has some static colors that
> were being used to initialize some other static variables in pcbnew's layer
> widgets. I have moved all the static colors to an initialize on first use
> paradigm (so now we just call them like a function, e.g.
> COLOR4D::UNSPECIFIED() ) to fix it.
>
> Also, this patch cleans up the layer toolbar button creation by moving all
> the statics into a member struct (this was done by Jeff as the fix in 5.1).
>
> -Ian
>
From 6f369328b3e4c3a66117a08d4e10736bfb081268 Mon Sep 17 00:00:00 2001
From: Ian McInerney
Date: Mon, 5 Aug 2019 10:51:29 +0200
Subject: [PATCH] Fix initialization of COLOR4D statics
The previous implementation would give an initialization order
fiasco since they were being used to initialize other statics.
This moves them to an initialize on first use paradigm.
---
.../3d_canvas/create_3Dgraphic_brd_items.cpp | 2 +-
common/board_printout.cpp | 4 ++--
common/colors_design_settings.cpp | 4 ++--
common/eda_draw_frame.cpp | 2 +-
common/eda_text.cpp | 2 +-
common/gal/cairo/cairo_gal.cpp| 2 +-
common/gal/color4d.cpp| 6 +
common/gal/opengl/antialiasing.cpp| 10
common/gal/opengl/opengl_compositor.cpp | 2 +-
common/gal/opengl/opengl_gal.cpp | 2 +-
common/gr_basic.cpp | 8 +++
common/plotters/DXF_plotter.cpp | 10 +++-
common/plotters/PS_plotter.cpp| 2 +-
common/plotters/common_plot_functions.cpp | 4 ++--
common/widgets/color_swatch.cpp | 4 ++--
common/widgets/layer_box_selector.cpp | 2 +-
eeschema/dialogs/dialog_edit_line_style.cpp | 6 ++---
.../dialogs/dialog_print_using_printer.cpp| 4 ++--
eeschema/eeschema.cpp | 6 ++---
eeschema/lib_text.cpp | 2 +-
eeschema/sch_line.cpp | 16 ++---
eeschema/sch_sheet.cpp| 2 +-
.../widgets/widget_eeschema_color_config.cpp | 6 ++---
gerbview/gbr_display_options.h| 2 +-
gerbview/gerbview_frame.cpp | 2 +-
gerbview/gerbview_layer_widget.cpp| 2 +-
gerbview/gerbview_painter.cpp | 2 +-
include/eda_draw_frame.h | 2 +-
include/gal/color4d.h | 24 +++
include/msgpanel.h| 2 +-
include/plotter.h | 4 ++--
...board_items_to_polygon_shape_transform.cpp | 2 +-
pcbnew/dialogs/dialog_pad_properties.cpp | 2 +-
pcbnew/exporters/export_vrml.cpp | 2 +-
pcbnew/exporters/gen_drill_report_files.cpp | 4 ++--
pcbnew/layer_widget.cpp | 6 ++---
pcbnew/layer_widget.h | 11 +
pcbnew/pad_print_functions.cpp| 6 ++---
pcbnew/pcb_layer_widget.cpp | 4 ++--
pcbnew/pcbnew_printout.cpp| 6 ++---
pcbnew/plot_board_layers.cpp | 8 ---
pcbnew/plot_brditems_plotter.cpp | 4 ++--
pcbnew/toolbars_pcb_editor.cpp| 10
pcbnew/tools/zone_create_helper.cpp | 2 +-
qa/eeschema/mocks_eeschema.cpp| 4 ++--
.../test_gal_pixel_alignment.cpp | 2 +-
46 files changed, 116 insertions(+), 105 deletions(-)
diff --git a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp
index 737297ef0..c3faeb9b2 100644
--- a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp
+++ b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp
@@ -106,7 +106,7 @@ void CINFO3D_VISU::AddShapeWithClearanceToContainer( const TEXTE_PCB* aText,
s_boardBBox3DU = _board2dBBox3DU;
// not actually used, but needed by GRText
-const COLOR4D dummy_color = COLOR4D::BLACK;
+const COLOR4D dummy_color = COLOR4D::BLACK();
if( aText->IsMultilineAllowed() )
{
diff --git a/common/board_printout.cpp b/common/board_printout.cpp
index dfc7e9725..7ab909090 100644
--- a/common/board_printout.cpp
+++ b/common/board_printout.cpp
@@ -114,7 +114,7 @@ void BOARD_PRINTOUT::DrawPage( const wxString& aLayerName, int aPageNum, int aPa
if( m_settings.m_blackWhite )
{
for( int i = 0; i < LAYER_ID_COUNT; ++i )
-dstSettings->SetLayerColor( i, COLOR4D::BLACK );
+
Re: [Kicad-developers] [Patch] Fix initialization order for COLOR4D statics
Hi, On Mon, Aug 05, 2019 at 10:58:44AM +0200, Ian McInerney wrote: > I tracked it down to the fact that COLOR4D has some static colors that were > being used to initialize some other static variables in pcbnew's layer > widgets. I have moved all the static colors to an initialize on first use > paradigm (so now we just call them like a function, e.g. > COLOR4D::UNSPECIFIED() ) to fix it. Can that be solved by constexpr? Simon ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
[Kicad-developers] [Patch] Fix initialization order for COLOR4D statics
I decided to instrument my debug build with the address sanitizer and see
what happened, and it has pointed out an initialization order fiasco in the
loading of pcbnew.
I tracked it down to the fact that COLOR4D has some static colors that were
being used to initialize some other static variables in pcbnew's layer
widgets. I have moved all the static colors to an initialize on first use
paradigm (so now we just call them like a function, e.g.
COLOR4D::UNSPECIFIED() ) to fix it.
Also, this patch cleans up the layer toolbar button creation by moving all
the statics into a member struct (this was done by Jeff as the fix in 5.1).
-Ian
From 3c63160bea386a08f169d0bde4680804fad037e1 Mon Sep 17 00:00:00 2001
From: Ian McInerney
Date: Mon, 5 Aug 2019 10:51:29 +0200
Subject: [PATCH] Fix initialization of COLOR4D statics
The previous implementation would give an initialization order
fiasco since they were being used to initialize other statics.
This moves them to an initialize on first use paradigm.
Also move the statics in the layer toolbar button creation into
a member struct.
---
.../3d_canvas/create_3Dgraphic_brd_items.cpp | 2 +-
common/board_printout.cpp | 4 ++--
common/colors_design_settings.cpp | 4 ++--
common/eda_draw_frame.cpp | 2 +-
common/eda_text.cpp | 2 +-
common/gal/cairo/cairo_gal.cpp| 2 +-
common/gal/color4d.cpp| 6 +
common/gal/opengl/antialiasing.cpp| 10
common/gal/opengl/opengl_compositor.cpp | 2 +-
common/gal/opengl/opengl_gal.cpp | 2 +-
common/gr_basic.cpp | 8 +++
common/plotters/DXF_plotter.cpp | 10 +++-
common/plotters/PS_plotter.cpp| 2 +-
common/plotters/common_plot_functions.cpp | 4 ++--
common/widgets/color_swatch.cpp | 4 ++--
common/widgets/layer_box_selector.cpp | 2 +-
eeschema/dialogs/dialog_edit_line_style.cpp | 6 ++---
.../dialogs/dialog_print_using_printer.cpp| 4 ++--
eeschema/eeschema.cpp | 6 ++---
eeschema/lib_text.cpp | 2 +-
eeschema/sch_line.cpp | 16 ++---
eeschema/sch_sheet.cpp| 2 +-
.../widgets/widget_eeschema_color_config.cpp | 6 ++---
gerbview/gbr_display_options.h| 2 +-
gerbview/gerbview_frame.cpp | 2 +-
gerbview/gerbview_layer_widget.cpp| 2 +-
gerbview/gerbview_painter.cpp | 2 +-
include/eda_draw_frame.h | 2 +-
include/gal/color4d.h | 24 +++
include/msgpanel.h| 2 +-
include/plotter.h | 4 ++--
...board_items_to_polygon_shape_transform.cpp | 2 +-
pcbnew/dialogs/dialog_pad_properties.cpp | 2 +-
pcbnew/exporters/export_vrml.cpp | 2 +-
pcbnew/exporters/gen_drill_report_files.cpp | 4 ++--
pcbnew/layer_widget.cpp | 6 ++---
pcbnew/layer_widget.h | 11 +
pcbnew/pad_print_functions.cpp| 6 ++---
pcbnew/pcb_layer_widget.cpp | 4 ++--
pcbnew/pcbnew_printout.cpp| 6 ++---
pcbnew/plot_board_layers.cpp | 8 ---
pcbnew/plot_brditems_plotter.cpp | 4 ++--
pcbnew/toolbars_pcb_editor.cpp| 10
pcbnew/tools/zone_create_helper.cpp | 2 +-
qa/eeschema/mocks_eeschema.cpp| 4 ++--
.../test_gal_pixel_alignment.cpp | 2 +-
46 files changed, 116 insertions(+), 105 deletions(-)
diff --git a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp
index 737297ef0..c3faeb9b2 100644
--- a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp
+++ b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp
@@ -106,7 +106,7 @@ void CINFO3D_VISU::AddShapeWithClearanceToContainer( const TEXTE_PCB* aText,
s_boardBBox3DU = _board2dBBox3DU;
// not actually used, but needed by GRText
-const COLOR4D dummy_color = COLOR4D::BLACK;
+const COLOR4D dummy_color = COLOR4D::BLACK();
if( aText->IsMultilineAllowed() )
{
diff --git a/common/board_printout.cpp b/common/board_printout.cpp
index dfc7e9725..7ab909090 100644
--- a/common/board_printout.cpp
+++ b/common/board_printout.cpp
@@ -114,7 +114,7 @@ void BOARD_PRINTOUT::DrawPage( const wxString& aLayerName, int aPageNum, int aPa
if( m_settings.m_blackWhite )
{
for( int i = 0; i < LAYER_ID_COUNT; ++i )
-dstSettings->SetLayerColor( i, COLOR4D::BLACK );
+dstSettings->SetLayerColor( i, COLOR4D::BLACK() );
}
else // color enabled
{
@@ -196,7 +196,7 @@ void BOARD_PRINTOUT::setupViewLayers( const std::unique_ptr& aView,
