[Kicad-developers] [PATCH 0/4] Use of "virtual" and "override" keywords
Hi, C++ has a misfeature where methods in derived classes can be virtual without that being immediately obvious, which makes class definitions a bit harder to read than strictly necessary because it isn't immediately obvious what is happening. In C++11, this is still the case, but at least we can use "override" specifiers in method declarations to mark overriding functions, and get a check from the compiler that this is indeed an override -- so if you are intending to write an override but forget a const-qualifier somewhere, you get a proper compilation error. This patchset - enables a warning on compilers that support this - removes the compiler flags override in the 3d_cache subdirectory (this causes all kinds of other interesting issues) - adds the "virtual" keyword on all implicitly virtual methods - adds the "override" specifier on all overriding methods Simon Simon Richter (4): Warn about missing override specifiers, if supported (gcc 5.1, clang 3.5) Don't clear compiler flags in 3d_cache directory Add "virtual" keyword on implicitly virtual methods Add C++11 "override" specifier to overrides 3d-viewer/3d_cache/CMakeLists.txt | 2 - 3d-viewer/3d_cache/dialogs/dlg_3d_pathconfig.h | 10 +- 3d-viewer/3d_cache/dialogs/dlg_select_3dmodel.h| 2 +- 3d-viewer/3d_cache/dialogs/panel_prev_model.h | 2 +- 3d-viewer/3d_cache/sg/scenegraph.h | 20 +- 3d-viewer/3d_cache/sg/sg_appearance.h | 20 +- 3d-viewer/3d_cache/sg/sg_colors.h | 20 +- 3d-viewer/3d_cache/sg/sg_coords.h | 20 +- 3d-viewer/3d_cache/sg/sg_faceset.h | 20 +- 3d-viewer/3d_cache/sg/sg_index.h | 20 +- 3d-viewer/3d_cache/sg/sg_normals.h | 20 +- 3d-viewer/3d_cache/sg/sg_shape.h | 20 +- 3d-viewer/3d_draw.cpp | 2 +- 3d-viewer/3d_material.h| 4 +- .../3d_render_ogl_legacy/c3d_render_ogl_legacy.h | 4 +- .../3d_render_raytracing/accelerators/ccontainer.h | 4 +- .../accelerators/ccontainer2d.h| 4 +- .../3d_rendering/3d_render_raytracing/cmaterial.h | 2 +- .../shapes2D/cfilledcircle2d.h | 10 +- .../3d_render_raytracing/shapes2D/cpolygon2d.h | 20 +- .../3d_render_raytracing/shapes2D/cpolygon4pts2d.h | 10 +- .../3d_render_raytracing/shapes2D/cring2d.h| 10 +- .../shapes2D/croundsegment2d.h | 10 +- .../3d_render_raytracing/shapes2D/ctriangle2d.h| 10 +- 3d-viewer/3d_rendering/ctrack_ball.h | 10 +- 3d-viewer/3d_struct.h | 6 +- 3d-viewer/3d_viewer.h | 4 +- 3d-viewer/dialogs/dialog_3D_view_option.cpp| 8 +- 3d-viewer/modelparsers.h | 4 +- CMakeLists.txt | 12 ++ bitmap2component/bitmap2cmp_gui.cpp| 22 +- common/confirm.cpp | 6 +- common/dialogs/dialog_image_editor.h | 18 +- common/dialogs/dialog_page_settings.h | 30 +-- common/dialogs/wx_html_report_panel.h | 14 +- common/single_top.cpp | 12 +- cvpcb/class_DisplayFootprintsFrame.h | 30 +-- cvpcb/cvpcb.cpp| 8 +- cvpcb/cvpcb_mainframe.h| 10 +- cvpcb/dialogs/dialog_config_equfiles.h | 14 +- cvpcb/dialogs/dialog_display_options.h | 8 +- cvpcb/dialogs/fp_conflict_assignment_selector.h| 10 +- cvpcb/listview_classes.h | 6 +- eeschema/class_libentry.h | 8 +- eeschema/class_sch_screen.h| 6 +- eeschema/dialogs/dialog_annotate.cpp | 6 +- eeschema/dialogs/dialog_bom.cpp| 20 +- eeschema/dialogs/dialog_choose_component.h | 18 +- eeschema/dialogs/dialog_edit_component_in_lib.h| 22 +- .../dialogs/dialog_edit_component_in_schematic.cpp | 24 +-- eeschema/dialogs/dialog_edit_label.cpp | 10 +- .../dialogs/dialog_edit_libentry_fields_in_lib.cpp | 20 +- eeschema/dialogs/dialog_edit_one_field.h | 12 +- eeschema/dialogs/dialog_eeschema_config.cpp| 20 +- eeschema/dialogs/dialog_eeschema_options.h | 10 +- eeschema/dialogs/dialog_erc.h | 14 +- eeschema/dialogs/dialog_lib_edit_pin.h | 12 +- eeschema/dialogs/dialog_lib_edit_pin_table.cpp | 38 ++-- eeschema/dialogs/dialog_lib_edit_pin_table.h | 2 +- eeschema/dialogs/dialog_lib_edit_text.h| 4 +- eeschema/dialogs/dialog_lib_new_component.h| 4 +- eeschema/dialogs/dialog_netlist.cpp| 16 +- eeschema/dialogs/dialog_plot_schematic.h | 10 +- eesche
Re: [Kicad-developers] [PATCH 0/4] Use of "virtual" and "override" keywords
Simon, I was looking over this patch set and before I apply it I want to be sure that all base class functions declared as virtual are explicitly declared as override in the derived classes. It's a very large patch set and I don't have time to confirm that this is the intent of these patches. Wayne On 5/1/2016 1:18 PM, Simon Richter wrote: > Hi, > > C++ has a misfeature where methods in derived classes can be virtual > without that being immediately obvious, which makes class definitions a bit > harder to read than strictly necessary because it isn't immediately obvious > what is happening. > > In C++11, this is still the case, but at least we can use "override" > specifiers in method declarations to mark overriding functions, and get a > check from the compiler that this is indeed an override -- so if you are > intending to write an override but forget a const-qualifier somewhere, you > get a proper compilation error. > > This patchset > > - enables a warning on compilers that support this > - removes the compiler flags override in the 3d_cache subdirectory >(this causes all kinds of other interesting issues) > - adds the "virtual" keyword on all implicitly virtual methods > - adds the "override" specifier on all overriding methods > >Simon > > Simon Richter (4): > Warn about missing override specifiers, if supported (gcc 5.1, clang > 3.5) > Don't clear compiler flags in 3d_cache directory > Add "virtual" keyword on implicitly virtual methods > Add C++11 "override" specifier to overrides > > 3d-viewer/3d_cache/CMakeLists.txt | 2 - > 3d-viewer/3d_cache/dialogs/dlg_3d_pathconfig.h | 10 +- > 3d-viewer/3d_cache/dialogs/dlg_select_3dmodel.h| 2 +- > 3d-viewer/3d_cache/dialogs/panel_prev_model.h | 2 +- > 3d-viewer/3d_cache/sg/scenegraph.h | 20 +- > 3d-viewer/3d_cache/sg/sg_appearance.h | 20 +- > 3d-viewer/3d_cache/sg/sg_colors.h | 20 +- > 3d-viewer/3d_cache/sg/sg_coords.h | 20 +- > 3d-viewer/3d_cache/sg/sg_faceset.h | 20 +- > 3d-viewer/3d_cache/sg/sg_index.h | 20 +- > 3d-viewer/3d_cache/sg/sg_normals.h | 20 +- > 3d-viewer/3d_cache/sg/sg_shape.h | 20 +- > 3d-viewer/3d_draw.cpp | 2 +- > 3d-viewer/3d_material.h| 4 +- > .../3d_render_ogl_legacy/c3d_render_ogl_legacy.h | 4 +- > .../3d_render_raytracing/accelerators/ccontainer.h | 4 +- > .../accelerators/ccontainer2d.h| 4 +- > .../3d_rendering/3d_render_raytracing/cmaterial.h | 2 +- > .../shapes2D/cfilledcircle2d.h | 10 +- > .../3d_render_raytracing/shapes2D/cpolygon2d.h | 20 +- > .../3d_render_raytracing/shapes2D/cpolygon4pts2d.h | 10 +- > .../3d_render_raytracing/shapes2D/cring2d.h| 10 +- > .../shapes2D/croundsegment2d.h | 10 +- > .../3d_render_raytracing/shapes2D/ctriangle2d.h| 10 +- > 3d-viewer/3d_rendering/ctrack_ball.h | 10 +- > 3d-viewer/3d_struct.h | 6 +- > 3d-viewer/3d_viewer.h | 4 +- > 3d-viewer/dialogs/dialog_3D_view_option.cpp| 8 +- > 3d-viewer/modelparsers.h | 4 +- > CMakeLists.txt | 12 ++ > bitmap2component/bitmap2cmp_gui.cpp| 22 +- > common/confirm.cpp | 6 +- > common/dialogs/dialog_image_editor.h | 18 +- > common/dialogs/dialog_page_settings.h | 30 +-- > common/dialogs/wx_html_report_panel.h | 14 +- > common/single_top.cpp | 12 +- > cvpcb/class_DisplayFootprintsFrame.h | 30 +-- > cvpcb/cvpcb.cpp| 8 +- > cvpcb/cvpcb_mainframe.h| 10 +- > cvpcb/dialogs/dialog_config_equfiles.h | 14 +- > cvpcb/dialogs/dialog_display_options.h | 8 +- > cvpcb/dialogs/fp_conflict_assignment_selector.h| 10 +- > cvpcb/listview_classes.h | 6 +- > eeschema/class_libentry.h | 8 +- > eeschema/class_sch_screen.h| 6 +- > eeschema/dialogs/dialog_annotate.cpp | 6 +- > eeschema/dialogs/dialog_bom.cpp| 20 +- > eeschema/dialogs/dialog_choose_component.h | 18 +- > eeschema/dialogs/dialog_edit_component_in_lib.h| 22 +- > .../dialogs/dialog_edit_component_in_schematic.cpp | 24 +-- > eeschema/dialogs/dialog_edit_label.cpp | 10 +- > .../dialogs/dialog_edit_libentry_fields_in_lib.cpp | 20 +- > eeschema/dialogs/dialog_edit_one_field.h | 12 +- > eeschema/dialogs/dialog_eeschema_config.cpp| 20 +- > eeschema/dialogs/dialog_eeschema_options.
Re: [Kicad-developers] [PATCH 0/4] Use of "virtual" and "override" keywords
Hi Wayne,
On 12.07.2016 16:07, Wayne Stambaugh wrote:
> I was looking over this patch set and before I apply it I want to be
> sure that all base class functions declared as virtual are explicitly
> declared as override in the derived classes. It's a very large patch
> set and I don't have time to confirm that this is the intent of these
> patches.
The patches will need to be rebased anyway, there have been several changes.
The first patch enables a warning for all functions that override
virtual functions in base classes but have no "override" -- that is how
I identified the implicitly virtual functions.
Whether we also add a "virtual" in addition to the "override" is a
question of code legibility, personally I find it more obvious to be
explicit here. It might also be an idea to place these in a separate
block inside the class definition, e.g.:
struct A {
virtual void a() = 0;
};
struct B : A {
virtual void b() = 0;
};
struct C : B {
// A
virtual void a() override;
// B
virtual void b() override;
};
This is what I do in my projects, and I've found it to be a tremendous
help, as it tells me that this function is likely to be called by code
that doesn't know the concrete type, but only the base class mentioned
in the comment above.
Simon
signature.asc
Description: OpenPGP digital signature
___
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 0/4] Use of "virtual" and "override" keywords
Why both "virtual" and "override" in "struct C"? It is redundant (you
cannot override a non-virtual function). See
http://en.cppreference.com/w/cpp/language/override.
Regards,
Cheng
On Wed, Jul 13, 2016 at 1:27 AM, Simon Richter
wrote:
> Hi Wayne,
>
> On 12.07.2016 16:07, Wayne Stambaugh wrote:
>
> > I was looking over this patch set and before I apply it I want to be
> > sure that all base class functions declared as virtual are explicitly
> > declared as override in the derived classes. It's a very large patch
> > set and I don't have time to confirm that this is the intent of these
> > patches.
>
> The patches will need to be rebased anyway, there have been several
> changes.
>
> The first patch enables a warning for all functions that override
> virtual functions in base classes but have no "override" -- that is how
> I identified the implicitly virtual functions.
>
> Whether we also add a "virtual" in addition to the "override" is a
> question of code legibility, personally I find it more obvious to be
> explicit here. It might also be an idea to place these in a separate
> block inside the class definition, e.g.:
>
> struct A {
> virtual void a() = 0;
> };
>
> struct B : A {
> virtual void b() = 0;
> };
>
> struct C : B {
> // A
> virtual void a() override;
>
> // B
> virtual void b() override;
> };
>
> This is what I do in my projects, and I've found it to be a tremendous
> help, as it tells me that this function is likely to be called by code
> that doesn't know the concrete type, but only the base class mentioned
> in the comment above.
>
>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
>
>
___
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 0/4] Use of "virtual" and "override" keywords
>It is redundant (you cannot override a non-virtual function) a() and b() in struct c in the example are virtual in both parents. override is simply a check to tell the compiler you intend to override a() and b() from parent classes. If you put virtual blah() in struct C without override, you can be creating a new virtual function if the signature does not match and its completely valid. ___ 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 0/4] Use of "virtual" and "override" keywords
On Thu, Jul 14, 2016 at 10:17:10AM -0400, Mark Roszko wrote: > If you put virtual blah() in struct C without override, you can be > creating a new virtual function if the signature does not match and > its completely valid. This happens to me about 99% of the times I change a function signature :D Love this new keyword -- Lorenzo Marcantonio CZ Srl - Parma signature.asc Description: PGP signature ___ 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 0/4] Use of "virtual" and "override" keywords
To Simon: Fair. It is indeed a little odd that C++ committee decides to put
one before but one after the signature. Only that you might want to write
it in the style guide; otherwise, both styles will appear in the future.
To Mark: I said {the use of both is redundant}, not {one of them is
redundant}. But if this scenario happens in code reviewing, I'd accept your
comment that the original statement isn't in its most exact form and I
should say "virtual is redundant when using virtual + override".
On Thu, Jul 14, 2016 at 4:17 PM, Mark Roszko wrote:
> >It is redundant (you cannot override a non-virtual function)
>
> a() and b() in struct c in the example are virtual in both parents.
>
>
> override is simply a check to tell the compiler you intend to override
> a() and b() from parent classes.
>
> If you put virtual blah() in struct C without override, you can be
> creating a new virtual function if the signature does not match and
> its completely valid.
>
___
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 0/4] Use of "virtual" and "override" keywords
Hi,
On 14.07.2016 16:17, Mark Roszko wrote:
> If you put virtual blah() in struct C without override, you can be
> creating a new virtual function if the signature does not match and
> its completely valid.
Right, but that is something we can catch with "override".
The reason I like to have them there, besides having it immediately
visible, is the case
struct A {
virtual void foo();
};
struct B : A {
void foo();
};
struct C : B {
void foo();
};
{
B *b = new C;
b->foo();
}
This calls C::foo(), until the signature of A::foo() changes, then it
starts calling B::foo().
Simon
signature.asc
Description: OpenPGP digital signature
___
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
