[Bf-committers] Code cleanup woes

Sergey Sharybin sergey.vfx at gmail.com
Mon Jan 28 17:49:46 CET 2013 

hrn, didn't we agreed to do clean=up in areas we're gonna to work further
just after this? Like cleanup some code if it prevents understanding what's
going on when fixing a bug and do not do a cleanup in code just to make it
cleaner.


On Mon, Jan 28, 2013 at 5:23 PM, Campbell Barton <ideasman42 at gmail.com>wrote:

> 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
> _______________________________________________
> 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