[Bf-committers] [Bf-blender-cvs] SVN commit: /data/svn/bf-blender [61066] trunk/blender/source/blender: code cleanup: warnings

[Sergey Sharybin]

That wouldn't be a problem if functions in blender didn't tend to be long
with loads of branches and returns from the middle in some cases.

I can see 6 allocations in bmo_wireframe_exec and all of them simply uses
__func__. I have no idea how you might solve possible memory leak happening
in such situation without going ahead and making IDs more unique.
Especially if Campbell is not around and we urgently need to solve issues
for artists ;)

If someone forgets to update allocation string after move/copypaste we need
to poke him.

But what about this. Make it so __func__ is always appending as a prefix to
string ID and you'll be passing var name (or even something more verbose)
to allocation functions? Some wrapper function will make it totally
transparent for all the cases.

- Extend MemHead with one char* field. We could remove mmap easily and
probably one of tags as well to prevent memory overhead.
- Pass __func__ to allocation functions as a separate argument.
- Add a macro wrapper which would look like: #define MEM_mallocN(size, str)
MEM_mallocN_ex(size, __func__, str)

Memleak prints will look like: "Unfreed block:
bmo_wireframe_exec():verts_src"

If someone could think of concatenation __func__ with str in a
preprocessor, that would make changes even simpler (__func__ is a static
char* so not sure it's possible).


On Sun, Nov 3, 2013 at 3:41 PM, Campbell Barton <ideasman42 at gmail.com>wrote:

> I've been using __func__ for simple cases:
>
> - where 1 or more arrays are allocated and freed within a single function.
>   (where the ID's wont show up in memory prints anyway - if they do
> its a mem leak in the function).
>
> - where a single array is allocated and returned from a function
> (where the function name is unique a useful reference).
>
> If there is some memory error - typically a direct link to the
> function is a good start to investigating the problem, from there its
> not so bad to edit an argument to differentiate (though in practice I
> can't recall having to do this).
>
> One of the reasons I prefer this is in the past, I kept running into
> string ID's that were based on vars or functions which were since
> renamed (or code was moved elsewhere), and nobody thought to update
> the names - so I'm happy with a reliable name even if its not always
> unique.
>
> note - that we don't attempt unique string ID's in all other parts of
> the code either.
>
> On Sun, Nov 3, 2013 at 8:25 PM, Sergey Sharybin <sergey.vfx at gmail.com>
> wrote:
> > Could see multiple allocations with the same block name of __func__. In
> > this particular case it's rather harmless (for until someone goes
> > initialize and adds return statement or so :), but i would rather be
> strict
> > about block names being unique named in general.
> >
> > Any good reason not using "bmo wireframe verts_{src,neg,pos}" instead of
> > __func__?
> >
> >
> > On Sun, Nov 3, 2013 at 11:19 AM, Campbell Barton <ideasman42 at gmail.com
> >wrote:
> >
> >> Revision: 61066
> >>
> >>
> http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-blender&revision=61066
> >> Author:   campbellbarton
> >> Date:     2013-11-03 05:19:55 +0000 (Sun, 03 Nov 2013)
> >> Log Message:
> >> -----------
> >> code cleanup: warnings
> >>
> >> Modified Paths:
> >> --------------
> >>     trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >>     trunk/blender/source/blender/editors/transform/transform.c
> >>
> >> Modified: trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >> ===================================================================
> >> --- trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >>  2013-11-02 23:40:39 UTC (rev 61065)
> >> +++ trunk/blender/source/blender/bmesh/operators/bmo_wireframe.c
> >>  2013-11-03 05:19:55 UTC (rev 61066)
> >> @@ -172,9 +172,9 @@
> >>         BMIter itersub;
> >>
> >>         /* filled only with boundary verts */
> >> -       BMVert **verts_src      = MEM_mallocN(sizeof(BMVert **) *
> >> totvert_orig, __func__);
> >> -       BMVert **verts_neg      = MEM_mallocN(sizeof(BMVert **) *
> >> totvert_orig, __func__);
> >> -       BMVert **verts_pos      = MEM_mallocN(sizeof(BMVert **) *
> >> totvert_orig, __func__);
> >> +       BMVert **verts_src      = MEM_mallocN(sizeof(BMVert *) *
> >> totvert_orig, __func__);
> >> +       BMVert **verts_neg      = MEM_mallocN(sizeof(BMVert *) *
> >> totvert_orig, __func__);
> >> +       BMVert **verts_pos      = MEM_mallocN(sizeof(BMVert *) *
> >> totvert_orig, __func__);
> >>
> >>         /* will over-alloc, but makes for easy lookups by index to keep
> >> aligned  */
> >>         BMVert **verts_boundary = use_boundary ?
> >>
> >> Modified: trunk/blender/source/blender/editors/transform/transform.c
> >> ===================================================================
> >> --- trunk/blender/source/blender/editors/transform/transform.c
>  2013-11-02
> >> 23:40:39 UTC (rev 61065)
> >> +++ trunk/blender/source/blender/editors/transform/transform.c
>  2013-11-03
> >> 05:19:55 UTC (rev 61066)
> >> @@ -4262,7 +4262,7 @@
> >>                 }
> >>         }
> >>         BLI_snprintf(str + ofs, MAX_INFO_LEN - ofs, IFACE_(" or Alt)
> Even
> >> Thickness %s"),
> >> -                    WM_bool_as_string(t->flag & T_ALT_TRANSFORM));
> >> +                    WM_bool_as_string((t->flag & T_ALT_TRANSFORM) !=
> 0));
> >>         /* done with header string */
> >>
> >>
> >> @@ -6946,7 +6946,7 @@
> >>                 }
> >>         }
> >>         ofs += BLI_snprintf(str + ofs, MAX_INFO_LEN - ofs, IFACE_(" or
> >> Alt) Expand to fit %s"),
> >> -                           WM_bool_as_string(t->flag &
> T_ALT_TRANSFORM));
> >> +                           WM_bool_as_string((t->flag &
> T_ALT_TRANSFORM)
> >> != 0));
> >>  }
> >>
> >>  static void applySeqSlideValue(TransInfo *t, const float val[2])
> >>
> >> _______________________________________________
> >> Bf-blender-cvs mailing list
> >> Bf-blender-cvs at blender.org
> >> http://lists.blender.org/mailman/listinfo/bf-blender-cvs
> >>
> >
> >
> >
> > --
> > With best regards, Sergey Sharybin
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers at blender.org
> > http://lists.blender.org/mailman/listinfo/bf-committers
>
>
>
> --
> - Campbell
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>



-- 
With best regards, Sergey Sharybin