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
