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

[Sergey Sharybin]

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