[Asterisk-code-review] stringfields: Refactor to allow fields to be added to the e... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: stringfields:  Refactor to allow fields to be added to the end of structures
......................................................................


Patch Set 7: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/2477/7/include/asterisk/stringfields.h
File include/asterisk/stringfields.h:

Line 167:   ast_calloc_with_stringfields_field_free(x, -1);
Left over ast_calloc_with_stringfields() change

ast_string_field_free_memory(x);
ast_free(x);


Line 320: enum ast_stringfield_cleanup_type {
How about these names:
AST_STRINGFIELD_CLEANUP_RESET -> AST_STRINGFIELD_RESET
AST_STRINGFIELD_CLEANUP_FREE_POOLS -> AST_STRINGFIELD_DESTROY


PS7, Line 333: 	/*!
             : 	 * Can only be used on a pointer allocated with ast_calloc_with_stringfields.
             : 	 * In addition to performing a FREE_POOLS, it also calls ast_free on the pointer.
             : 	 */
             : 	AST_STRINGFIELD_CLEANUP_FREE_ALL = -2,
Remove this.  This is an internal implementation detail.


Line 386:  * \brief Free sting field
s/sting/string/


PS7, Line 394: * \note
             :  * If ast_string_field_free returns -1 when called with a pointer returned by
             :  * ast_calloc_with_stringfields(), you'll need to free the memory yourself with ast_free.
no longer applies


Line 398: #define ast_string_field_free(x, cleanup_type)	 \
This new API call isn't really necessary as its functionality is covered by ast_string_field_init() and ast_string_field_free_memory().


PS7, Line 405: 		if (!__res__ && __embedded_pool__ && cleanup_type == AST_STRINGFIELD_CLEANUP_FREE_ALL) { \
             : 			ast_free((x)); \
             : 		} \
Because this code is inlined everywhere, this adds code that is only useful for pointers created with ast_calloc_with_stringfields().  For the majority of cases this is dead code.

ast_calloc_with_strigfields() is used as a convenience that combined allocation and stringfield initialization in one step nothing more.


Line 449:  * \param n Number of structures to allocate (see ast_calloc)
Current implementation only allows 1 to be used here.


https://gerrit.asterisk.org/#/c/2477/7/main/stringfields.c
File main/stringfields.c:

Line 167: 	*pool_head = preserve;
Need to set
if (preserve) {
   preserve->prev = NULL;
}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I235db338c5b178f5a13b7946afbaa5d4a0f91d61
Gerrit-PatchSet: 7
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes