Is there any advantage of such coding-style?

Question

|There is this function in gestremer matroska demux plugin:

gboolean
gst_matroska_demux_plugin_init (GstPlugin * plugin)
{
  /* parser helper separate debug */
  GST_DEBUG_CATEGORY_INIT (ebmlread_debug, "ebmlread",
      0, "EBML stream helper class");

  /* create an elementfactory for the matroska_demux element */
  if (!gst_element_register (plugin, "matroskademux",
          GST_RANK_PRIMARY, GST_TYPE_MATROSKA_DEMUX))
    return FALSE;

  return TRUE;
}

Now gst_element_register() is type of

gboolean            gst_element_register                (GstPlugin *plugin,
                                                         const gchar *name,
                                                         guint rank,
                                                         GType type);
Returns :
    TRUE, if the registering succeeded, FALSE on error

Then why not write it in the following way?

 gboolean
    gst_matroska_demux_plugin_init (GstPlugin * plugin)
    {
      /* parser helper separate debug */
      GST_DEBUG_CATEGORY_INIT (ebmlread_debug, "ebmlread",
          0, "EBML stream helper class");

      /* create an elementfactory for the matroska_demux element */
      return gst_element_register (plugin, "matroskademux",
              GST_RANK_PRIMARY, GST_TYPE_MATROSKA_DEMUX))      
    }

Answer 1

I would go out on a limb and just say outright that yes, the latter style is superior. That is:

if (!func())
   return FALSE;
return TRUE;

... and assuming func() already returns a boolean, the above style is inferior to:

return func();

If the former seems more readable, I'd implore those who think so to get a better understanding of expressions. If the argument is that it better emphasizes that this function returns TRUE or FALSE, that would mean the reader would not know how func() works which should be understood by those looking at the function and trying to really understand the logic and not just shotgun debugging random bits of code.

That said, this tends to happen in practice as multiple developers maintain the code, as people modify the code temporarily to make debugging easier, etc.

I wouldn't consider the presence of such code to be a problem, but I would assert that the concise solution is better as it guarantees that it is at least as efficient or more than the longer one which implies additional overhead which we can only hope for the compiler to optimize away (and I would not expect that to happen with these kinds of branching cases for all compilers even with how good optimizing compilers are nowadays).

Answer 2

Basically, no: it uses more code and more instructions to say the same thing.

Usually, this indicates one of two things:

• someone who isn't very comfortable with C
• code where someone planned to do something more with the negative return, eg, log an error message, but didn't.

Hm. In this case it may indicate code translated mechanically from, say, Fortran.

Updated again

Okay, so this is still controversial. Here's an actual example. Observe the example is exactly isomorphic to the OP example:

C code:

int retcod1() { return 0; }

int ex1(){
    if(retcod1())
        return 0;
    else
        return 1;
}

int ex2() {
    return retcod1();
}

Generated Assembly:

This is the code as generated with gcc -S -O0:

    .file   "code.c"
    .text
.globl retcod1
    .type   retcod1, @function
retcod1:
.LFB0:
    .cfi_startproc
    pushq   %rbp
    .cfi_def_cfa_offset 16
    movq    %rsp, %rbp
    .cfi_offset 6, -16
    .cfi_def_cfa_register 6
    movl    $0, %eax
    leave
    .cfi_def_cfa 7, 8
    ret
    .cfi_endproc
.LFE0:
    .size   retcod1, .-retcod1
.globl ex1
    .type   ex1, @function
ex1:
.LFB1:
    .cfi_startproc
    pushq   %rbp
    .cfi_def_cfa_offset 16
    movq    %rsp, %rbp
    .cfi_offset 6, -16
    .cfi_def_cfa_register 6
    movl    $0, %eax
    call    retcod1
    testl   %eax, %eax
    je  .L3
    movl    $0, %eax
    jmp .L4
.L3:
    movl    $1, %eax
.L4:
    leave
    .cfi_def_cfa 7, 8
    ret
    .cfi_endproc
.LFE1:
    .size   ex1, .-ex1
.globl ex2
    .type   ex2, @function
ex2:
.LFB2:
    .cfi_startproc
    pushq   %rbp
    .cfi_def_cfa_offset 16
    movq    %rsp, %rbp
    .cfi_offset 6, -16
    .cfi_def_cfa_register 6
    movl    $0, %eax
    call    retcod1
    leave
    .cfi_def_cfa 7, 8
    ret
    .cfi_endproc
.LFE2:
    .size   ex2, .-ex2
    .ident  "GCC: (SUSE Linux) 4.5.1 20101208 [gcc-4_5-branch revision 167585]"
    .section    .comment.SUSE.OPTs,"MS",@progbits,1
    .string "ospwg"
    .section    .note.GNU-stack,"",@progbits

For convenience (assuming SO can handle the format) I've taken the generated code for the two routines and put them side by side. Observe that the second example is significantly shorter.

.globl ex1                           .globl ex2                       
    .type   ex1, @function          .type   ex2, @function   
ex1:                                 ex2:                             
.LFB1:                               .LFB2:                           
    .cfi_startproc                  .cfi_startproc           
    pushq   %rbp                    pushq   %rbp             
    .cfi_def_cfa_offset 16          .cfi_def_cfa_offset 16   
    movq    %rsp, %rbp              movq    %rsp, %rbp       
    .cfi_offset 6, -16              .cfi_offset 6, -16       
    .cfi_def_cfa_register 6         .cfi_def_cfa_register 6  
    movl    $0, %eax                movl    $0, %eax         
    call    retcod1                 call    retcod1          
    testl   %eax, %eax              leave                    
    je  .L3                     .cfi_def_cfa 7, 8        
    movl    $0, %eax                ret                      
    jmp .L4                     .cfi_endproc             
.L3:                                 .LFE2:                           
    movl    $1, %eax
.L4:
    leave
    .cfi_def_cfa 7, 8
    ret
    .cfi_endproc
.LFE1:
    .size   ex1, .-ex1

Here's an example with default optimization, ie, gcc -S:

ex1:                                      ex2:                              
.LFB1:                                    .LFB2:                            
    .cfi_startproc                      .cfi_startproc            
    pushq   %rbp                        pushq   %rbp              
    .cfi_def_cfa_offset 16              .cfi_def_cfa_offset 16    
    movq    %rsp, %rbp                  movq    %rsp, %rbp        
    .cfi_offset 6, -16                  .cfi_offset 6, -16        
    .cfi_def_cfa_register 6             .cfi_def_cfa_register 6   
    movl    $0, %eax                    movl    $0, %eax          
    call    retcod1                     call    retcod1           
    testl   %eax, %eax                  leave                     
    je  .L3                         .cfi_def_cfa 7, 8         
    movl    $0, %eax                    ret                       
    jmp .L4                         .cfi_endproc              
.L3:                                      .LFE2:                            
    movl    $1, %eax                    .size   ex2, .-ex2        
.L4:
    leave
    .cfi_def_cfa 7, 8
    ret
    .cfi_endproc
.LFE1:
    .size   ex1, .-ex1
.globl ex2
    .type   ex2, @function

Again, still significantly shorter.

Finally, here's an example, this time with full optimization:

.globl ex1                            .globl ex2                         
    .type   ex1, @function          .type   ex2, @function     
ex1:                                  ex2:                               
.LFB1:                                .LFB2:                             
    .cfi_startproc                  .cfi_startproc             
    movl    $1, %eax                xorl    %eax, %eax         
    ret                             ret                        
    .cfi_endproc                    .cfi_endproc               
.LFE1:                                .LFE2:                             

Now, observe that even with full optimization on, it still doesn't create exactly the same code, as was asserted.

Answer 3

In addition to what has already been said, and since this is tagged coding style, the first code contains some style that could be considered dangerous. But since it is coding style, opinions about it will be subjective.

• Multiple return statements is considered bad and dangerous style by many programmers.
• if without following braces is considered bad and dangerous style by many programmers. Same thing for else, else if, for, while, do...while, switch.

Both of these styles are banned by MISRA-C, for example ( MISRA-C:2004 14.7 and 14.8).

(Personally I don't agree with the former of those two, I think there are many cases where multiple return statements make the code more readable, particularly in functions where a lot of error checking takes place, such as parsers etc.)

Answer 4

As such there is no problem with the code. At least I wont penalize if anyone uses any of the mentioned snippets.

These are the reasons that I think are the cause:

• The foremost is readability. Now looking at the section code, I know what is success and what is not. As there could be apis, which behaves inversely (even in C).
• Second reason happens, when during testing you want to print some log statements. Now if you return directly you cannot log the return value or any debug info. You can say after code is freed, one should remove it, but then for next release you might have to put debug statements again. So it stays there.
• Third reason could be that some code is removed during development.

Conclusion is what I said in the beginning. It does not matter as long as code is easy to understand. Regarding some optimization gains, I think for this compilers are intelligent enough to take care.

Answer 5

It's part of a pattern.

if (!some_function(...))
    return false;
if (!other_function(...))
    return false;

return true;

Whoever wrote it decided not to change the pattern just because there's only one function call. Ultimately it's a matter of taste.