[Bf-committers] Disputed #define abuse... STACK_INIT etc

Campbell Barton ideasman42 at gmail.com
Sat Jun 1 23:58:56 CEST 2013


On Sat, Jun 1, 2013 at 11:10 PM, Ton Roosendaal <ton at blender.org> wrote:
> Hi Campbell,
>
> I can imagine it's "your" code, but I can't follow such code. Before it was quite readable, now it's something obsfucated.

This isn't my code, it was written by Joe before bmesh merge, He used
BLI_array_ macros a lot since it made code easy to update (which I
dont blame him for), but in cases where fixed sized arrays are
adequate, they are overkill.
And can now be replaced with BLI_buffers which implement array
resizing without macros.

> http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-blender&revision=57062
>
> I thought we should try to limit use of #defines globally in code?
> Further, optimizing is always asking for troubles...

Yep, however this is just replacing one set of defines with another
(BLI_array_*** are all macros too).

> And now that code crashes (still):
> http://projects.blender.org/tracker/index.php?func=detail&aid=35585&group_id=9&atid=498

As mentioned before, this code basically only worked by accident
because BLI_array_*** functions over allocate.

> Crashes in array magic is always really hard to debug. The saner the code the better.

For sure, but check this out!


---------------
Original Code:

	/* fill newl with destination vertex indices */
	mv = cddm->mvert;
	c = 0;
	for (i = 0; i < dm->numVertData; i++, mv++) {
		if (vtargetmap[i] == -1) {
			BLI_array_append(oldv, i);
			newv[i] = c++;
			BLI_array_append(mvert, *mv);
		}
		else {
			/* dummy value */
			newv[i] = 0;
		}
	}


---------------
Original Code (macro Expanded):


 mv = cddm->mvert;
 c = 0;
 for (i = 0; i < dm->numVertData; i++, mv++) {
  if (vtargetmap[i] == -1) {
   ( (void) (( (((void *)(oldv) == ((void *)0)) &&
	            ((void *)(_oldv_static) != ((void *)0)) &&
	            ((sizeof(_oldv_static) / sizeof(*oldv)) >= _oldv_count + 1)) ?
	               (void)(oldv = (void *)_oldv_static) :
	               (__builtin_expect(!!(( (size_t) (((void *)(oldv) ==
(void *)_oldv_static &&
	                                                 (void *)(oldv) !=
((void *)0)) ?
	
(sizeof(_oldv_static) / sizeof(*oldv)) :
	                                                    ( ((oldv) ==
((void *)0)) ? 0 : MEM_allocN_len(oldv) / sizeof(*oldv) )) )
	                                    >= _oldv_count + 1), 1) ?
	                    (void)0 : _bli_array_grow_func((void **)&(oldv),
_oldv_static, sizeof(*oldv), _oldv_count, 1, "BLI_array." "oldv"),
(void)0) ),
	         (_oldv_count += 1)), (void) (oldv[_oldv_count - 1] = i) );
   newv[i] = c++;
   ( (void) (( (((void *)(mvert) == ((void *)0)) &&
                ((void *)(_mvert_static) != ((void *)0)) &&
                ((sizeof(_mvert_static) / sizeof(*mvert)) >=
_mvert_count + 1)) ?
                   (void)(mvert = (void *)_mvert_static) :
                   (__builtin_expect(!!(( (size_t) (((void *)(mvert)
== (void *)_mvert_static &&
                                                     (void *)(mvert)
!= ((void *)0)) ?

(sizeof(_mvert_static) / sizeof(*mvert)) :
                                                        ( ((mvert) ==
((void *)0)) ? 0 : MEM_allocN_len(mvert) / sizeof(*mvert) )))
                                        >= _mvert_count + 1), 1) ?
                        (void)0 : _bli_array_grow_func((void **)&(mvert),
                                                       _mvert_static,
sizeof(*mvert), _mvert_count, 1, "BLI_array." "mvert"), (void)0) ),
             (_mvert_count += 1)), (void) (mvert[_mvert_count - 1] = *mv) );
  }
  else {

   newv[i] = 0;
  }
 }


---------------
New Code (macro Expanded):

 mv = cddm->mvert;
 c = 0;
 for (i = 0; i < totvert; i++, mv++) {
  if (vtargetmap[i] == -1) {
   (void)((oldv)[(_oldv_index)++] = i);
   (void)((mvert)[(_mvert_index)++] = *mv);
   newv[i] = c++;
  }
  else {

   newv[i] = 0;
  }
 }

----------------------

As for being obsfucated.
IMHO old and new are about the same if you ignore macro expansion stuff,
but having to debug these macros (Which I've had to do on multiple
occasions)  - I can assure you isn't fun.


More information about the Bf-committers mailing list