[Asterisk-code-review] vector: Traversal, retrieval, insert and locking enhancements (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: vector:  Traversal, retrieval, insert and locking enhancements
......................................................................


Patch Set 3: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/339/3/include/asterisk/vector.h
File include/asterisk/vector.h:

Line 415: #define AST_RWVECTOR_REMOVE_ELEM_UNORDERED(vec, elem, cleanup) ({ \
Nit-pick: I think we can do without the enclosing ({ }) for this since we only calling a single macro.  This applies to all other REMOVE_ELEM macros.  I know the existing code had these in ({}), up to you if you want to ignore this comment.


Line 496: 	if (ast_rwlock_rdlock(&(vec)->lock) == 0) { \
Nit-pick: Some places you test !ast_rwlock_rdlock, others you test == 0.  Personally I prefer !ast_rwlock_rdlock, but being consistent either way within this header would be nice.


Line 513: #define AST_VECTOR_CALLBACK(vec, callback, arg) ({ \
        : 	size_t idx; \
        : 	for (idx = 0; idx < (vec)->current; idx++) { \
        : 		int rc = callback((vec)->elems[idx], arg, 0);	\
        : 		if (rc == CMP_STOP) { \
        : 			idx++; \
        : 			break; \
        : 		}\
        : 	} \
        : 	idx; \
        : })
I don't see the benefit to forcing use of ao2_callback style function for vector callbacks.  We're using macro's, lets take advantage.
#define AST_VECTOR_CALLBACK(vec, callback, ...)

In the loop run: callback((vec)->elems[idx], ##__VA_ARGS__)

This way if someone wants to use an ao2_callback function they can run AST_VECTOR_CALLBACK(vec, cb, arg, 0), or they can do whatever is most useful.


Line 537: 	if (ast_rwlock_wrlock(&(vec)->lock) == 0) { \
rdlock.


Line 599: 	if (ast_rwlock_wrlock(&(vec)->lock) == 0) { \
rdlock.


https://gerrit.asterisk.org/#/c/339/3/tests/test_vector.c
File tests/test_vector.c:

Line 210: 	test_validate_cleanup(AST_VECTOR_SIZE(&sv1) == 0);
Even if locking is not required for a certain operation, I think we should still have an AST_RWVECTOR_SIZE(vec).  This is useful so code reviewers can easily see that all actions on AST_RWVECTOR's use the correct macro variants.

I do think we need read or write locking for every operation that reads or writes vec.  Applies to every use of AST_VECTOR_* in this test.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2e07ecc709d2f5f91bcab8904e5e9340609b00e0
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes