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

[wdoekes]

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

Ship it!


Apart from my minor comments, I see no glaring errors.


/branches/1.8/main/xmldoc.c
<https://reviewboard.asterisk.org/r/1485/#comment8661>

    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.



/branches/1.8/main/xmldoc.c
<https://reviewboard.asterisk.org/r/1485/#comment8662>

    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.


- wdoekes


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/5092d0bc/attachment-0001.htm>