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 <[email protected]>wrote: > On Mon, Jan 28, 2013 at 9:46 PM, Emil Brink <[email protected]> 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 <[email protected] > >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 <[email protected]> > 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 [email protected] > 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 <[email protected]> > >> 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 [email protected] > >> www.blender.org > >> >>> Blender Institute Entrepotdok 57A 1018AD Amsterdam The > Netherlands > >> >>> > >> >>> _______________________________________________ > >> >>> Bf-committers mailing list > >> >>> [email protected] > >> >>> http://lists.blender.org/mailman/listinfo/bf-committers > >> >> _______________________________________________ > >> >> Bf-committers mailing list > >> >> [email protected] > >> >> http://lists.blender.org/mailman/listinfo/bf-committers > >> > > >> > _______________________________________________ > >> > Bf-committers mailing list > >> > [email protected] > >> > http://lists.blender.org/mailman/listinfo/bf-committers > >> > >> > >> > >> -- > >> - Campbell > >> _______________________________________________ > >> Bf-committers mailing list > >> [email protected] > >> http://lists.blender.org/mailman/listinfo/bf-committers > >> > > _______________________________________________ > > Bf-committers mailing list > > [email protected] > > http://lists.blender.org/mailman/listinfo/bf-committers > > > > -- > - Campbell > _______________________________________________ > Bf-committers mailing list > [email protected] > http://lists.blender.org/mailman/listinfo/bf-committers > -- With best regards, Sergey Sharybin _______________________________________________ Bf-committers mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
