[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 5: Code-Review-1

(10 comments)

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

Line 319: #define AST_STRING_FIELD_EXTENDED(name) ast_string_field name
> Because AST_STRING_FIELD is const and AST_STRING_FIELD_EXTENDED can't be in
It really should be made the same and then do casting inside the initialization.  Otherwise, it is a difference between normal and extended.  Also external code should not be able to alter stringfields.


Line 351: 	if (((void *)(x)) != NULL) { \
> It's needed if you pass the address of an auto structure.
Ugh.  Sometimes the compiler is too stupid for its own good. :)


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

Line 159: 	 * to free the structure that contains the stringfield manager
> Actually looking at this again, I'm not sure if needed = 0 ever worked corr
The original function set all stringfields to the empty string before doing anything else.  For this pathway, the AST_VECTOR_CALLBACK_VOID() does this so it should be left here.


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

Line 115: 	size_t initial_vector_size = ((size_t) (((char *)mgr) - ((char *)p))) / sizeof(*p);
I'm not sure if this is correct.  It might be off by one but I cannot verify at this time.


PS5, Line 163: 	if ((needed < 0) && mgr->header->embedded_pool) {
             : 		needed = 0;
             : 	}
             : 
             : 	if (needed < 0) {		/* reset all pools */
Something still isn't quite right on when the additional memory pieces need to be freed and when they shouldn't.  I'm not sure how to fix at this time.

needed isn't always going to be < 0 because it is set to zero in the highlighted code.

Same with the needed < 0 check to free header below.


Line 467: 	if (AST_VECTOR_SIZE(left) != AST_VECTOR_SIZE(right)) {
Comparing apples and oranges structures.  Shouldn't we just assert this check?  Do you know of a case where we are comparing different structs?


PS5, Line 484: 	struct ast_string_field_vector *v1 = &(copy_mgr->header->string_fields);
             : 	struct ast_string_field_vector *v2 = &(orig_mgr->header->string_fields);
Better names come to mind
v1 is dest
v2 is src


Line 487: 	if (AST_VECTOR_SIZE(v1) != AST_VECTOR_SIZE(v2)) {
Same here


Line 496: 		if (ast_string_field_ptr_set_by_fields(copy_pool, *copy_mgr, AST_VECTOR_GET(v1, i), *AST_VECTOR_GET(v2, i))) {
long line :)


https://gerrit.asterisk.org/#/c/2477/4/tests/test_stringfields.c
File tests/test_stringfields.c:

Line 327: 	case TEST_EXECUTE:
> Never worked with number of structures set to anything other than 1 because
Well we could create a C++ delete[] like function that you pass the array and the same calloc parameters used to create it.  Kind of moot though since it is never used that way.


-- 
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: 5
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