[Bf-committers] Code cleanup woes

Campbell Barton ideasman42 at gmail.com
Mon Jan 28 12:23:35 CET 2013 

On Mon, Jan 28, 2013 at 9:46 PM, Emil Brink <emil at obsession.se> wrote:
> Obligatory verbiage triggered by the code Campbell quoted:
>
> Please consider not ever casting the return value of malloc() (and other
> allocation functions that all return void *) in C.
>
> Doing so can mask actual errors, doesn't do anything useful, and often also
> clutters the code since the cast expression can be unwieldy.
>
> Pointer types convert to/from void * in C without issue (the exception is
> function pointers, but you can't malloc() a function so that's moot).
>
> Also, use sizeof more, and realize it's an operator (not a function).
> Instead of the brittle and hard-to-read
>
> temp = (AviIndexEntry *) MEM_mallocN ((frame_num+1) *
> (movie->header->Streams+1) * sizeof(AviIndexEntry),"newidxentry");
>
> write:
>
> temp = MEM_mallocN((frame_num + 1) * (movie->header->Streams + 1) * sizeof
> *temp, "newidxentry");
>
> This "locks" the allocation to sized according to the size of the data
> pointed at by temp, which is better than repeating the type name (which
> could be wrong).
>
> Of course, you all know how much code I contribute to Blender, so obviously
> I don't have much of a say in this matter. Just trying to help. :)
>
> Regards,
>
> /Emil

interesting, blender is casting alloc's a lot, grep for:
\(\s*[A-z0-9_]+\s*\*+\s*\)\s*MEM_[m|c]allocN
shows up 348 hits.

but this is a trap! since it involves more code cleanups :).

I'll leave this for now.



> On Sun, Jan 27, 2013 at 1:24 PM, Campbell Barton <ideasman42 at gmail.com>wrote:
>
>> Of course this is not a "fix", but as our API's change, it makes sense
>> (sometimes) point to use the improved API's in existing code.
>>
>>
>> It came to my attention that quite a bit of our code was doing
>> (calloc/memcpy/free) just to resize an existing array.
>>
>> With re-calloc you can do...
>> ---
>> temp = (AviIndexEntry *) MEM_recallocN(movie->entries, (frame_num + 1)
>> * entry_size);
>> ---
>>
>> Compared to old code.
>> ---
>>         temp = (AviIndexEntry *) MEM_mallocN ((frame_num+1) *
>>             (movie->header->Streams+1) *
>> sizeof(AviIndexEntry),"newidxentry");
>>         if (movie->entries != NULL) {
>>                 memcpy (temp, movie->entries, movie->index_entries *
>> (movie->header->Streams+1)
>>                     * sizeof(AviIndexEntry));
>>                 MEM_freeN (movie->entries);
>>         }
>> ---
>> When looking into a bug report in areas of code like this you may want
>> to double check the sizeof(), offsets, alloc-size etc is correct (off
>> by one cause uninitialized memory of writing out of memory bounds), so
>> even if its working its more code to wade through and check from time
>> to time.
>>
>> Not to say going over and replacing all code is something we should
>> do, but with an API that makes some common expression more concise - I
>> believe it can be good to do so with care (and testing of course,
>> which I did, just not adding from zero).
>> Devs use existing code as a reference, so if we use our API's in a
>> clear way I think it helps us in the long term with GSOC, patches -
>> new modules.
>>
>> IMHO we should try to have our own code be an example of what we would
>> like others to develop in blender.
>>
>> In practice its a compromise, some refactors/cleanups are just too
>> disruptive, error-prone or time consuming.
>>
>> But I disagree with Ton's comment: "New code should be totally and
>> undeniably better, for users. Not for coders."
>>
>> Coders have to read/debug blenders existing code a _lot_. - Reducing
>> confusion is important and reduces errors we make when we do end up
>> making functional changes to the code (really! - quite a few bugs in
>> the tracker I found have been caused by devs who improve blender but
>> miss checking for non obvious side-effects or just having trouble with
>> confusingly written code).
>>
>> In the case of our particle code, I agree that we can just leave it
>> and not try do more cleanup - just maintain until its re-written.
>>
>> On Sun, Jan 27, 2013 at 8:29 PM, Ton Roosendaal <ton at blender.org> wrote:
>> > Hi,
>> >
>> > You miss the point Jason. It's not about being stupid or not thinking
>> enough.
>> > It shouldn't even have come up in a coders mind to "fix" it.
>> >
>> > -Ton-
>> >
>> > ------------------------------------------------------------------------
>> > Ton Roosendaal  Blender Foundation   ton at blender.org    www.blender.org
>> > Blender Institute   Entrepotdok 57A  1018AD Amsterdam   The Netherlands
>> >
>> > On 26 Jan, 2013, at 22:02, Jason Wilkins wrote:
>> >
>> >> I think this would not have broken if MEM_reallocN was exactly like
>> >> the standard library realloc.  The slight differences probably mislead
>> >> whoever was making this change.
>> >>
>> >> On Sat, Jan 26, 2013 at 6:42 AM, Ton Roosendaal <ton at blender.org>
>> wrote:
>> >>> Hi all,
>> >>>
>> >>> Here's another nice illustration of code cleanup that caused bugs;
>> >>>
>> http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-blender&revision=54058
>> >>>
>> >>> The idea of "replace calloc + memcpy with recalloc" sounds nice, but
>> it breaks the case when memcpy is of length zero. (zero hairs, use 'add'
>> brush in particle mode).
>> >>>
>> >>> May I quote Joelonsoftware here?
>> >>>
>> >>> "Old code has been used. It has been tested. Lots of bugs have been
>> found, and they've been fixed. There's nothing wrong with it. It doesn't
>> acquire bugs just by sitting around on your hard drive. Au contraire, baby!"
>> >>>
>> >>> Great fun read:
>> >>> http://www.joelonsoftware.com/articles/fog0000000069.html
>> >>>
>> >>> Of course we should never be afraid to recode parts of Blender, but it
>> shouldn't be with the naive assumption that the old bad messy code wasn't
>> functional. New code should be totally and undeniably better, for users.
>> Not for coders. :)
>> >>>
>> >>> We can moan and complain about our current bad particle code a lot,
>> but it has offered a lot of good for Blender for many years. The challenge
>> is not to make a new system with clean design, but to make a new system
>> that's far superior.
>> >>>
>> >>> -Ton-
>> >>>
>> >>>
>> ------------------------------------------------------------------------
>> >>> Ton Roosendaal  Blender Foundation   ton at blender.org
>> www.blender.org
>> >>> Blender Institute   Entrepotdok 57A  1018AD Amsterdam   The Netherlands
>> >>>
>> >>> _______________________________________________
>> >>> 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
>> >
>> > _______________________________________________
>> > 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
>>
> _______________________________________________
> 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