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

[Matthew Nicholson]

> On Oct. 6, 2011, 1:57 a.m., wdoekes wrote:
> > /branches/1.8/main/xmldoc.c, lines 522-525
> > <https://reviewboard.asterisk.org/r/1485/diff/2/?file=21002#file21002line522>
> >
> >     Why did you remove the ast_xml_node_get_children check? I don't mind, but if it is false one day, you have changed behaviour.
> >     
> >     However, for proper behaviour, such a check would now be better in the while() above, so first_match and lang_match can be ignored as well if they are childless.

I removed it because it is done elsewhere. I considered adding it in the loop, and I think that's what I will do instead of removing it.


> 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.
> 
> wdoekes wrote:
>     Haha.. I'm missing an:
>     
>     } else {
>        break;
>     }
>     
>     there at the end now though ;)

Noted.


- Matthew


-----------------------------------------------------------
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/9caf8d70/attachment-0001.htm>