[Asterisk-code-review] pbx: Create pbx include.c for management of 'struct ast incl... (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: pbx: Create pbx_include.c for management of 'struct ast_include'.
......................................................................


Patch Set 2:

> >"Although ast_walk_context_includes is maintained the procedure is
 > no longer efficient except for the first call (inc==NULL)."
 > 
 > Is the inefficiency a real concern for calling scenarios? Also
 > curious as to why the switch to a vector in the first place? Is it
 > warranted if their are efficiency issues?

It is used on call path's by app_while and app_macro.  The concern is that each call to ast_walk_context_includes starts from idx=0.  That said I'm not sure how much it matters that we rescan the vector of pointers on each call to walk, unless someone has hundreds or thousands of includes to one context.  There is no string comparison so I'd guess it's minor.

The main goal with switching to vectors is for 'ast_include' to be opaque to all other objects (especially ast_context).  FYI it's just the one call that is inefficient because it was designed specifically for linked lists.

 > > "I'm unsure the best approach to efficiently provide this
 > functionality as it would require an API change."
 > 
 > Would this work: Add an index variable to the ast_include
 > structure? Then ast_walk_context_includes would just be a call to
 > vector get at the given inc->index + 1. The main problem I see here
 > is upon an element being removed and the vector gets reordered.
 > However in the 'walk' function a reset of the next element's index
 > could be done if it differs more than +1 from the previous one so
 > the next call wouldn't potentially skip elements. Thoughts?

It's the callers responsibility to keep the contexts locked while using the ast_walk_context_* functions, so we do not need to worry about items being removed during walk.  I'm not a huge fan of maintaining an index for each include - the main goal of this patch is to isolate ast_include so I'd rather it not know it's place in the list.

Right now I'm thinking of creating two procedures:
int ast_context_include_count(struct ast_context *con);
struct ast_include *ast_context_include_get(struct ast_context *con, int idx);

All ast_walk_context_* functions combined are called from a total of 27 lines, so switching to vector style iteration would be low impact, provide the best efficiency.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5c882e27cf96fb2aec67a39c18b4c71c9c83b60
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: No