[Bf-committers] Code cleanup woes
Emil Brink
emil at obsession.se
Mon Jan 28 11:46:11 CET 2013
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
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
>
More information about the Bf-committers
mailing list