[Bf-committers] [Bf-blender-cvs] SVN commit: /data/svn/bf-blender [61066] trunk/blender/source/blender: code cleanup: warnings
Sergey Sharybin
sergey.vfx at gmail.com
Sun Nov 3 12:27:09 CET 2013
Fine with postponing this for after git migration.
We could either leave this as is (if strings are modifying by actual
maintainers, not during code cleanup by other devs). Would also be nice to
have some guidelines in wiki anyway.
But ok, let's finish with git first and then improve code :)
On Sun, Nov 3, 2013 at 5:12 PM, Campbell Barton <ideasman42 at gmail.com>wrote:
> We could just leave maintainers to choose - which is what we do currently.
>
> Some devs have...
> ar = MEM_callocN(sizeof(ARegion), "area region from do_versions");
>
> Other times the id's are numbered...
>
> jstart = (int *)MEM_mallocN(sizeof(float) * totv, "makeNurbfaces4");
> jend = (int *)MEM_mallocN(sizeof(float) * totv, "makeNurbfaces5");
>
> I did a quick check over bmesh code (where I've used this), and IMHO
> this is not causing confusion or problems - I've done quite a lot of
> debugging for BMesh and really this is not a pain point - even when
> I've had to troubleshoot leaks.
>
>
> Irrespective - I'd rather postpone this for now until after git merge.
>
> On Sun, Nov 3, 2013 at 9:58 PM, Sergey Sharybin <sergey.vfx at gmail.com>
> wrote:
> > I run couple of times when i needed to modify IDs to figure out which
> exact
> > allocation wasn't freed. And i rather make it so code uses unique IDs
> from
> > the first place, not only when you actually run into memory leak. Not
> > regarding to bmesh or motrack, all the code is to follow the same rules.
> >
> > It's really not a big deal to make strings a bit more unique. It's more
> > 3sec to type when you work on the code first time, and it saves loads of
> > time when someone needs to find which exact allocation wasn't freed
> (making
> > IDs unique, re-compiling, not speaking of white hairs :)
> >
> > So if there're no objections, i would just go ahead implementing proposal
> > above. Think this will solve both our needs :) Hopefully extra arg is not
> > gonna to affect on performance.
> >
> >
> > On Sun, Nov 3, 2013 at 4:46 PM, Campbell Barton <ideasman42 at gmail.com
> >wrote:
> >
> >> This is probably more a matter of personal preference, you have a
> >> point, but I just never found this to be a problem - if it comes it it
> >> you could replace all occurances of `__func__` with `AT` and get line
> >> numbers for each alloc.
> >>
> >> The reason Id rather not use `AT` is it gives really long ID's (full
> >> path to source:line, and it doesnt group well in guarded alloc
> >> prints).
> >>
> >> A lot of the bmesh functions are really simple WRT allocations - and
> >> dont have a lot of complex branching with returns, just alloc some
> >> arrays, do some calculations, free and exit, the're really not hard to
> >> debug memory wise --- where most problems come from memory being used
> >> and freed by different functions. For these cases I'm all for having
> >> more descriptive ID's for each memory block.
> >>
> >> On Sun, Nov 3, 2013 at 9:44 PM, Sergey Sharybin <sergey.vfx at gmail.com>
> >> wrote:
> >> > Just got another idea. Use AT instead of __func__. This way we don't
> need
> >> > extra arguments to allocation functions and only need small wrapper
> for
> >> > them.
> >> >
> >> >
> >> > On Sun, Nov 3, 2013 at 4:14 PM, Sergey Sharybin <sergey.vfx at gmail.com
> >> >wrote:
> >> >
> >> >> 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
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > 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
> > _______________________________________________
> > 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
More information about the Bf-committers
mailing list