[Asterisk-code-review] vector: Additional enhancements and fixes (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: vector:  Additional enhancements and fixes
......................................................................


Patch Set 1:

(6 comments)

https://gerrit.asterisk.org/#/c/374/1/include/asterisk/vector.h
File include/asterisk/vector.h:

Line 134: 	ast_free((vec)->elems);			\
        : 	(vec)->elems = NULL;			\
        : 	(vec)->max = 0;				\
        : 	(vec)->current = 0;			\
> Shouldn't this just invoke AST_VECTOR_FREE, then call ast_free?
Done


Line 163: 	ast_rwlock_destroy(&(vec)->lock); \
Invoke AST_VECTOR_RW_FREE() then ast_free(vec).


Line 254: #define AST_VECTOR_INSERT_AT(vec, idx, elem) ({ \
        : 	int res = 0; \
        : 	size_t __move; \
        : 	do { \
        : 		if ((vec)->current + 1 > (vec)->max) { \
        : 			size_t new_max = (vec)->max ? 2 * (vec)->max : 1; \
        : 			typeof((vec)->elems) new_elems = ast_realloc( \
        : 				(vec)->elems, new_max * sizeof(*new_elems)); \
        : 			if (new_elems) { \
        : 				(vec)->elems = new_elems; \
        : 				(vec)->max = new_max; \
        : 			} else { \
        : 				res = -1; \
        : 				break; \
        : 			} \
        : 		} \
        : 		if ((vec)->current > 0) { \
        : 			__move = ((vec)->current - 1) * sizeof(typeof((vec)->elems[0])); \
        : 			memmove(&(vec)->elems[(idx) + 1], &(vec)->elems[(idx)], __move); \
        : 		} \
        : 		(vec)->elems[(idx)] = (elem); \
        : 		(vec)->current++; \
        : 	} while (0); \
        : 	res; \
        : })
There are a few other problems with this definition.
1) Resizing the vector needs to use the same method as AST_VECTOR_REPLACE() and not ast_realloc.
2) What if the given idx is beyond current?
3) The memmove() to shift elements is sized to shift ALL but one vector element.  It needs to shift elements idx to current up one.


Line 486:  * \brief Execute a callback on every element in a vector returning the last matched
This is not how ao2_callback works.

ao2_callback returns the first matched element and stops traversal unless OBJ_MULTIPLE is set.


Line 497: 	typeof((vec)->elems[0]) res = NULL;				\
This only works if the vector elements are pointers.  The elements could be int or struct.  Probably should pass a default element value.


Line 543:  * AST_VECTOR(, char *) vector_1;
        :  * typeof(vector_1) *vector_2 = NULL;
        :  *
        :  * vector_2 = AST_VECTOR_CALLBACK_MULTIPLE(&vector_1, callback);
Probably should not recommend this type of coding outside of macros since typeof is a gcc extension.

The other way to get two variables with the same type is:
AST_VECTOR(, char *) vector_1, *vector_2;

This is the only exception for one variable declaration per line since the type is defined at the same time.

struct {
 int a;
 int b;
} var1, *var2;


-- 
To view, visit https://gerrit.asterisk.org/374
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I05e5e47fd02f61964be13b7e8942bab5d61b29cc
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes