[svn-commits] mmichelson: branch mmichelson/npm_fixes r180854 - /team/mmichelson/npm_fixes/...

[SVN commits to the Digium repositories]

Author: mmichelson
Date: Tue Mar 10 14:04:57 2009
New Revision: 180854

URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=180854
Log:
Add some of my notes I've made as comments.

Most of these will probably be removed once I'm done with this
branch, but I imagine some will remain but be modified since they
help to understand the code better.


Modified:
    team/mmichelson/npm_fixes/main/pbx.c

Modified: team/mmichelson/npm_fixes/main/pbx.c
URL: http://svn.digium.com/svn-view/asterisk/team/mmichelson/npm_fixes/main/pbx.c?view=diff&rev=180854&r1=180853&r2=180854
==============================================================================
--- team/mmichelson/npm_fixes/main/pbx.c (original)
+++ team/mmichelson/npm_fixes/main/pbx.c Tue Mar 10 14:04:57 2009
@@ -1911,6 +1911,8 @@
 				} else if (*s1 == '-') { /* remember to add some error checking to all this! */
 					char s3 = *(s1-1);
 					char s4 = *(s1+1);
+					/* XXX MCM - This seems wrong. Why increment s3 at the beginning of the for loop?
+					 */
 					for (s3++; s3 <= s4; s3++) {
 						*s2++ = s3;
 					}
@@ -1950,6 +1952,11 @@
 		}
 		m2 = 0;
 		if (already && (m2=already_in_tree(m1,buf)) && m2->next_char) {
+			/* XXX MCM - I think it makes sense to check if m2->exten is non-NULL. If it is
+			 * then it means we've found a duplicate extension. In this particular scenario, what
+			 * we do is overwrite what was previously in m2 with what we are now parsing. At least
+			 * this weird logic is consistent (see further down).
+			 */
 			if (!(*(s1+1))) {  /* if this is the end of the pattern, but not the end of the tree, then mark this node with the exten...
 								a shorter pattern might win if the longer one doesn't match */
 				m2->exten = e1;
@@ -1958,22 +1965,47 @@
 			m1 = m2->next_char; /* m1 points to the node to compare against */
 			m0 = &m2->next_char; /* m0 points to the ptr that points to m1 */
 		} else { /* not already OR not m2 OR nor m2->next_char */
+			/* XXX MCM - if (m2) means that 'already' is true and that this exten already
+			 * exists in the trie. It also means that m2->next_char is NULL, meaning we have
+			 * found a duplicate extension. Logic appears to show that we point the new
+			 * match_char to the existing one. If we're at the end of the current exten, then
+			 * we will end up overwriting the exten currently in m2
+			 */
 			if (m2) {
 				if (findonly)
 					return m2;
 				m1 = m2; /* while m0 stays the same */
+			/* XXX MCM - This else means that m2 is NULL. This could mean that either 'already'
+			 * is false or that m2 did not already exist in the tree. In either case it means
+			 * that we are dealing with a pattern not previously existing in the tree and we
+			 * need to add a new node to the tree. Adding a new node does not necessarily indicate
+			 * that we are adding a new exten.
+			 */
 			} else {
 				if (findonly)
 					return m1;
 				m1 = add_pattern_node(con, m1, buf, pattern, already,specif, m0); /* m1 is the node just added */
+				/* XXX We should be sure to make sure that m1 is non-NULL here
+				 */
 				m0 = &m1->next_char;
 			}
 			
+			/* XXX MCM - This specifically sees if we have reached the end of the
+			 * exten we are parsing. If so, then set m1->exten to e1. Note that if this
+			 * is an exact duplicate of an already existing pattern, then we are overwriting
+			 * m2->exten here and so the old ast_exten will not be reachable in the trie. We do
+			 * not print any sort of warning about this, and I think we should. Also, it would
+			 * be a smart idea to make sure that m1 is non-NULL here since add_pattern_node could
+			 * return NULL.
+			 */
 			if (!(*(s1+1))) {
 				m1->deleted = 0;
 				m1->exten = e1;
 			}
 			
+			/* The 'already' variable is a mini-optimization designed to make it so that we
+			 * don't have to call already_in_tree when we know it will return false.
+			 */
 			already = 0;
 		}
 		s1++; /* advance to next char */