[Bf-committers] [Bf-blender-cvs] SVN commit: /data/svn/bf-blender [61066] trunk/blender/source/blender: code cleanup: warnings
Campbell Barton
ideasman42 at gmail.com
Sun Nov 3 12:12:42 CET 2013
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
More information about the Bf-committers
mailing list