[asterisk-dev] [Code Review]: Load the proper XML documentation when multiple modules document the same application

[wdoekes]

> On Oct. 6, 2011, 1:57 a.m., wdoekes wrote:
> > /branches/1.8/main/xmldoc.c, lines 501-514
> > <https://reviewboard.asterisk.org/r/1485/diff/2/?file=21002#file21002line501>
> >
> >     This could be replaced with:
> >     
> >     if (!ast_strlen_zero(module)) {
> >       int match;
> >       attr = ast_xml_get_attribute(node, "module");
> >       match = (attr && !strcmp(attr, module));
> >       ast_xml_free_attr(attr); // <-- already checks attr for NULL
> >       if (match) {
> >         break;
> >       }
> >     }
> >     
> >     (1) This way you don't have 2 free's. In my experience, keeping allocs and frees paired (not having a free for every branch) reduces the chance of memleaks vastly.
> >     (2) The node=ast_xml_node_get_next() is called if we continue without break'ing. Letting if(match) break instead gets us the same correct behaviour.

Haha.. I'm missing an:

} else {
   break;
}

there at the end now though ;)


- wdoekes


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1485/#review4483
-----------------------------------------------------------


On Oct. 5, 2011, 12:09 p.m., Matthew Nicholson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1485/
> -----------------------------------------------------------
> 
> (Updated Oct. 5, 2011, 12:09 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds an option "module" attribute to the XML documentation spec that allows the documentation processor to match apps with identical names from different modules to their documentation. This patch also fixes a number of bugs with the documentation processor and should make it a little more efficient (and support for multiple languages should actually work now).
> 
> 
> This addresses bug ASTERISK-18130.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18130
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_fax.c 339509 
>   /branches/1.8/doc/appdocsxml.dtd 339509 
>   /branches/1.8/include/asterisk/module.h 339509 
>   /branches/1.8/include/asterisk/xmldoc.h 339509 
>   /branches/1.8/main/loader.c 339509 
>   /branches/1.8/main/manager.c 339509 
>   /branches/1.8/main/pbx.c 339509 
>   /branches/1.8/main/xmldoc.c 339509 
>   /branches/1.8/res/res_agi.c 339509 
>   /branches/1.8/res/res_fax.c 339509 
> 
> Diff: https://reviewboard.asterisk.org/r/1485/diff
> 
> 
> Testing
> -------
> 
> Tested on my test box. Works and doesn't break anything.
> 
> 
> Thanks,
> 
> Matthew
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20111006/a8d362f8/attachment.htm>