[Bf-committers] Code cleanup rules
Sergey Sharybin
sergey.vfx at gmail.com
Thu May 1 08:20:40 CEST 2014
Aha, the thread has been already started here. Would summarize my two
messages here.
Doing an if-defed inclusion of own files based not only the platform but
also based on compiler and it's version is just STUPID. I do want to hear a
weighful point on this, and not just "to reduce amount of re-compile time
after the cleanup".
The point is, if some .c|.cc|.cpp file needs some function it IS to include
a header file here this function is declared and NEVER rely on indirect
inclusion. I simply don't believe in cases when function is only used on
windoze from this snippet:
#ifdef WIN32
# include "BLI_string.h" /* BLI_strcasecmp */
#endif
So please do stick to a rule "include all you're using and no more than
this in the implementation file". With all this changes you've just
increased "WTF IS GOING ON??" factor when reading the affected files.
Further, with all this compiler-specific includes you've made it so only
compilers you're compiled will work. Did you test armel architecture? Did
you test hurd kernel? Did you test BSD after all? I do believe with all
this "cleanup" some of the platforms might easily became broken.
And one last thing, please DO NOT do such a cleanup silently from .au while
maintainers of all other platforms are sleeping because of timezone lag.
Such things are to be discussed first and i would tell you my negative
feeling about ifdef-ed includes on the early stages.
On Thu, May 1, 2014 at 3:38 AM, Chad Fraleigh <chadf at triularity.org> wrote:
> Just a style question..
>
> Wouldn't it be better for the BLI_*.h files to do the WIN32 (or other)
> ifdefs and otherwise be used platform generic, rather than make each C file
> that uses them do the ifdef?
>
> -Chad
>
>
> On Wed, Apr 30, 2014 at 2:27 PM, Thomas Dinges <blender at dingto.org> wrote:
>
> > Hi,
> > Next time you do such massive cleanup, you can also just spend 10mins of
> > your time and boot your Windows installation before commit.
> > I spend 30min of my time now to fix the issues, and then you revert that
> > 5minutes afterwards. Very nice.
> >
> > I clearly vote for more strict rules in regards to code cleanups that
> > affect all areas/and OS. This madness has to stop.
> >
> > Am 30.04.2014 23:21, schrieb Campbell Barton:
> > > Commit: af86b008b2a3dacc33b4987c50d8ffa24f6f9817
> > > Author: Campbell Barton
> > > Date: Thu May 1 07:21:08 2014 +1000
> > >
> https://developer.blender.org/rBaf86b008b2a3dacc33b4987c50d8ffa24f6f9817
> > >
> > > Include removal gave problems with windows, ifdef some back in for
> > windows only
> > >
> > > ===================================================================
> > >
> > > M source/blender/blenkernel/intern/customdata.c
> > > M source/blender/blenkernel/intern/ipo.c
> > > M source/blender/blenlib/intern/path_util.c
> > > M source/blender/blenlib/intern/smallhash.c
> > > M source/blender/bmesh/intern/bmesh_mesh.c
> > > M source/blender/editors/interface/interface_style.c
> > > M source/blender/editors/screen/area.c
> > > M source/blender/editors/space_outliner/outliner_tree.c
> > > M source/blender/editors/space_view3d/view3d_ops.c
> > > M source/blender/imbuf/intern/allocimbuf.c
> > > M source/blender/python/intern/bpy_interface.c
> > > M source/blender/python/intern/bpy_rna_array.c
> > > M source/blender/python/intern/bpy_traceback.c
> > >
> > > ===================================================================
> > >
> > > diff --git a/source/blender/blenkernel/intern/customdata.c
> > b/source/blender/blenkernel/intern/customdata.c
> > > index fd43501..57758ac 100644
> > > --- a/source/blender/blenkernel/intern/customdata.c
> > > +++ b/source/blender/blenkernel/intern/customdata.c
> > > @@ -63,6 +63,10 @@
> > > #include <math.h>
> > > #include <string.h>
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_alloca.h"
> > > +#endif
> > > +
> > > /* number of layers to add when growing a CustomData object */
> > > #define CUSTOMDATA_GROW 5
> > >
> > > diff --git a/source/blender/blenkernel/intern/ipo.c
> > b/source/blender/blenkernel/intern/ipo.c
> > > index 6f7ee31..7385322 100644
> > > --- a/source/blender/blenkernel/intern/ipo.c
> > > +++ b/source/blender/blenkernel/intern/ipo.c
> > > @@ -77,6 +77,10 @@
> > >
> > > #include "MEM_guardedalloc.h"
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_math_base.h" /* M_PI */
> > > +#endif
> > > +
> > > /* *************************************************** */
> > > /* Old-Data Freeing Tools */
> > >
> > > diff --git a/source/blender/blenlib/intern/path_util.c
> > b/source/blender/blenlib/intern/path_util.c
> > > index 2c24a89..f207329 100644
> > > --- a/source/blender/blenlib/intern/path_util.c
> > > +++ b/source/blender/blenlib/intern/path_util.c
> > > @@ -50,6 +50,8 @@
> > > #include "GHOST_Path-api.h"
> > >
> > > #ifdef WIN32
> > > +# include "MEM_guardedalloc.h"
> > > +
> > > # include "utf_winfunc.h"
> > > # include "utfconv.h"
> > > # include <io.h>
> > > diff --git a/source/blender/blenlib/intern/smallhash.c
> > b/source/blender/blenlib/intern/smallhash.c
> > > index d6b2383..e8e3387 100644
> > > --- a/source/blender/blenlib/intern/smallhash.c
> > > +++ b/source/blender/blenlib/intern/smallhash.c
> > > @@ -56,6 +56,10 @@
> > >
> > > #include "BLI_utildefines.h"
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_alloca.h"
> > > +#endif
> > > +
> > > #include "BLI_smallhash.h"
> > >
> > > #include "BLI_strict_flags.h"
> > > diff --git a/source/blender/bmesh/intern/bmesh_mesh.c
> > b/source/blender/bmesh/intern/bmesh_mesh.c
> > > index e9d3c36..6b040ef 100644
> > > --- a/source/blender/bmesh/intern/bmesh_mesh.c
> > > +++ b/source/blender/bmesh/intern/bmesh_mesh.c
> > > @@ -40,6 +40,10 @@
> > > #include "BKE_editmesh.h"
> > > #include "BKE_multires.h"
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_alloca.h"
> > > +#endif
> > > +
> > > #include "intern/bmesh_private.h"
> > >
> > > /* used as an extern, defined in bmesh.h */
> > > diff --git a/source/blender/editors/interface/interface_style.c
> > b/source/blender/editors/interface/interface_style.c
> > > index bbdfd1d..fa31c20 100644
> > > --- a/source/blender/editors/interface/interface_style.c
> > > +++ b/source/blender/editors/interface/interface_style.c
> > > @@ -55,6 +55,9 @@
> > >
> > > #include "interface_intern.h"
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_math_base.h" /* M_PI */
> > > +#endif
> > >
> > > /* style + theme + layout-engine = UI */
> > >
> > > diff --git a/source/blender/editors/screen/area.c
> > b/source/blender/editors/screen/area.c
> > > index 151764d..05659f6 100644
> > > --- a/source/blender/editors/screen/area.c
> > > +++ b/source/blender/editors/screen/area.c
> > > @@ -68,6 +68,10 @@
> > >
> > > #include "screen_intern.h"
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_alloca.h"
> > > +#endif
> > > +
> > > extern void ui_draw_anti_tria(float x1, float y1, float x2, float y2,
> > float x3, float y3); /* xxx temp */
> > >
> > > /* general area and region code */
> > > diff --git a/source/blender/editors/space_outliner/outliner_tree.c
> > b/source/blender/editors/space_outliner/outliner_tree.c
> > > index bfa0dcd..76a0839 100644
> > > --- a/source/blender/editors/space_outliner/outliner_tree.c
> > > +++ b/source/blender/editors/space_outliner/outliner_tree.c
> > > @@ -77,6 +77,10 @@
> > >
> > > #include "outliner_intern.h"
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_math_base.h" /* M_PI */
> > > +#endif
> > > +
> > > /* ********************************************************* */
> > > /* Persistent Data */
> > >
> > > diff --git a/source/blender/editors/space_view3d/view3d_ops.c
> > b/source/blender/editors/space_view3d/view3d_ops.c
> > > index 4fa995b..a8128ba 100644
> > > --- a/source/blender/editors/space_view3d/view3d_ops.c
> > > +++ b/source/blender/editors/space_view3d/view3d_ops.c
> > > @@ -57,6 +57,10 @@
> > >
> > > #include "view3d_intern.h"
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_math_base.h" /* M_PI */
> > > +#endif
> > > +
> > > /* ************************** copy paste
> *****************************
> > */
> > >
> > > static int view3d_copybuffer_exec(bContext *C, wmOperator *op)
> > > diff --git a/source/blender/imbuf/intern/allocimbuf.c
> > b/source/blender/imbuf/intern/allocimbuf.c
> > > index ff9bef7..6125177 100644
> > > --- a/source/blender/imbuf/intern/allocimbuf.c
> > > +++ b/source/blender/imbuf/intern/allocimbuf.c
> > > @@ -48,6 +48,7 @@
> > > #include "MEM_guardedalloc.h"
> > >
> > > #include "BLI_threads.h"
> > > +#include "BLI_utildefines.h"
> > >
> > > static SpinLock refcounter_spin;
> > >
> > > diff --git a/source/blender/python/intern/bpy_interface.c
> > b/source/blender/python/intern/bpy_interface.c
> > > index 90cc07d..43ca695 100644
> > > --- a/source/blender/python/intern/bpy_interface.c
> > > +++ b/source/blender/python/intern/bpy_interface.c
> > > @@ -37,6 +37,10 @@
> > >
> > > #include <Python.h>
> > >
> > > +#ifdef WIN32
> > > +# include "BLI_math_base.h" /* finite */
> > > +#endif
> > > +
> > > #include "MEM_guardedalloc.h"
> > >
> > > #include "BLI_utildefines.h"
> > > diff --git a/source/blender/python/intern/bpy_rna_array.c
> > b/source/blender/python/intern/bpy_rna_array.c
> > > index a7a3c49..033f8a3 100644
> > > --- a/source/blender/python/intern/bpy_rna_array.c
> > > +++ b/source/blender/python/intern/bpy_rna_array.c
> > > @@ -30,11 +30,12 @@
> > >
> > > #include "RNA_types.h"
> > >
> > > -
> > > #include "bpy_rna.h"
> > > #include "BKE_global.h"
> > > #include "MEM_guardedalloc.h"
> > >
> > > +#include "BLI_utildefines.h"
> > > +
> > > #include "RNA_access.h"
> > >
> > > #define USE_MATHUTILS
> > > diff --git a/source/blender/python/intern/bpy_traceback.c
> > b/source/blender/python/intern/bpy_traceback.c
> > > index a917421..7ae6d3a 100644
> > > --- a/source/blender/python/intern/bpy_traceback.c
> > > +++ b/source/blender/python/intern/bpy_traceback.c
> > > @@ -31,6 +31,9 @@
> > >
> > > #include "BLI_utildefines.h"
> > > #include "BLI_path_util.h"
> > > +#ifdef WIN32
> > > +# include "BLI_string.h" /* BLI_strcasecmp */
> > > +#endif
> > >
> > > #include "bpy_traceback.h"
> > >
> > > _______________________________________________
> > > Bf-blender-cvs mailing list
> > > Bf-blender-cvs at blender.org
> > > http://lists.blender.org/mailman/listinfo/bf-blender-cvs
> >
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers at blender.org
> > http://lists.blender.org/mailman/listinfo/bf-committers
> >
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>
--
With best regards, Sergey Sharybin
More information about the Bf-committers
mailing list