[asterisk-dev] [Code Review] 4547: clang compiler warning: braces-around-scalar-initializer

[Diederik de Groot]

> On March 28, 2015, 3:59 p.m., Matt Jordan wrote:
> > /branches/13/channels/chan_iax2.c, lines 7674-7676
> > <https://reviewboard.asterisk.org/r/4547/diff/1/?file=73110#file73110line7674>
> >
> >     I really dislike this warning.
> >     
> >     In C, it has always been perfectly valid to initialize all members of a struct using { 0 } as a universal zero-initializer. This nomenclature actually makes it less readable, as it makes it look like we only wanted to initialize the .frametype member, and ignored the rest. That may not be the actual effect, but the syntax here is not clearer by any stretch.
> >     
> >     :-(
> >     
> >     Interestingly, when compiling with clang 3.4, I don't get this warning issued. What version are you compiling with? And what do you think about simply ignoring this warning?

Using "struct ast_frame f = { 0, };" is perfectly fine and does not raise an error

However: "struct ast_frame f = { AST_FRAME_CONTROL, { AST_CONTROL_CONGESTION } };" however does raise this warning, which does make a little more sense. 

I updated all of these (i hope i got them all) so that they all use the same syntax. In case someone ones to update/change/extend one of them.

FYI: There is one major benefit in using the named variety, namely allowing you to change the order of the fields in the struct without consequence. For example the ast_frame struct could be optimized a little by reordering the fields to improve packing, as in: 

    struct ast_frame {
        /*! Kind of frame */
        enum ast_frame_type frametype;
        /*! Length of data */
        int datalen;
        /*! Subclass, frame dependent */
        struct ast_frame_subclass subclass;
        /*! Number of samples in this frame */
        int samples;
        /*! Was the data malloc'd?  i.e. should we free it when we discard the frame? */
        int mallocd;
        /*! The number of bytes allocated for a malloc'd frame header */
        size_t mallocd_hdr_len;
        /*! How many bytes exist _before_ "data" that can be used if needed */
        int offset;
        /*! Misc. frame flags */
        unsigned int flags;
        /*! Optional source of frame for debugging */
        const char *src;
        /*! Pointer to actual data */
        union { void *ptr; uint32_t uint32; char pad[8]; } data;
        /*! Global delivery time */
        struct timeval delivery;
        /*! For placing in a linked list */
        AST_LIST_ENTRY(ast_frame) frame_list;
        /*! Timestamp in milliseconds */
        long ts;
        /*! Length in milliseconds */
        long len;
        /*! Sequence number */
        int seqno;
    };

would get rid of some of the padding (on x86_64).

Note: this warning might have occured after playing with the struct layout of ast_frame to be honest (Need to recheck this). If so this change should be considered an enhancement.
Note2: I compile using clang-3.6


- Diederik


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


On March 27, 2015, 7:34 p.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4547/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 7:34 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24917
>     https://issues.asterisk.org/jira/browse/ASTERISK-24917
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs. clang compiler warning:braces-around-scalar-initializer
> 
> 
> Diffs
> -----
> 
>   /branches/13/res/res_pjsip_dtmf_info.c 433444 
>   /branches/13/channels/sig_ss7.c 433444 
>   /branches/13/channels/sig_pri.c 433444 
>   /branches/13/channels/pjsip/dialplan_functions.c 433444 
>   /branches/13/channels/console_gui.c 433444 
>   /branches/13/channels/chan_unistim.c 433444 
>   /branches/13/channels/chan_skinny.c 433444 
>   /branches/13/channels/chan_sip.c 433444 
>   /branches/13/channels/chan_oss.c 433444 
>   /branches/13/channels/chan_mgcp.c 433444 
>   /branches/13/channels/chan_iax2.c 433444 
>   /branches/13/channels/chan_dahdi.c 433444 
>   /branches/13/channels/chan_console.c 433444 
>   /branches/13/channels/chan_alsa.c 433444 
>   /branches/13/apps/app_dictate.c 433444 
> 
> Diff: https://reviewboard.asterisk.org/r/4547/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150328/b9412e65/attachment.html>