HalosGhost / shaman

A small, native C library and utility to fetch weather
GNU General Public License v2.0
49 stars 4 forks source link

take into account that memory allocation may fail #3

Closed mhogomchungu closed 10 years ago

HalosGhost commented 10 years ago

I feel like this is overkill. There really isn't that much mallocing going on, so I think it would be easier if we just surrounded the few that are used in conditional assignments, it would be significantly more efficient. E.g.,

if ( !(malloc(whatever)) )
{   fprintf(stderr, "Could not allocate memory\n");
    // do other clean-up
    exit(1);
}

This, I think, would address the possibility of a failure to malloc while not going overboard with seemingly useless extra functions. Thoughts?

mhogomchungu commented 10 years ago

you most likely will never run out of memory but its good practice to take into account the possibility.

The compiler will not generate function calls to "_malloc()" and "_realloc()" since they are declared as static and they are small and hence the code of these functions will be "put in place" where these functions are called giving you no overhead of function calls while giving you code clarity through less conditional checks.

below is the generated assembly code that shows the body of "_realloc()" function was substituted inside "_writeDataToBuffer" and hence there was no function call to "_realloc()" meaning there is no inefficiency introduced but the "_writeDataToBuffer()" function look "nice" since it has one less conditional check..

You dont have to understand assembly to see what i just said,just look through it and the absence of "_realloc" and presence of "realloc" should be enough to prove what i just said above.

Declaring functions as static prevents unnecessary pollution of global namespace but it also allows the compiler to do optimizations by removing function calls.


08048e68 <_writeDataToBuffer>:
 8048e68:   55                      push   %ebp
 8048e69:   89 e5                   mov    %esp,%ebp
 8048e6b:   57                      push   %edi
 8048e6c:   56                      push   %esi
 8048e6d:   53                      push   %ebx
 8048e6e:   83 ec 24                sub    $0x24,%esp
 8048e71:   8b 75 08                mov    0x8(%ebp),%esi
 8048e74:   8b 55 10                mov    0x10(%ebp),%edx
 8048e77:   8b 5d 14                mov    0x14(%ebp),%ebx
 8048e7a:   0f af 55 0c             imul   0xc(%ebp),%edx
 8048e7e:   8b 43 04                mov    0x4(%ebx),%eax
 8048e81:   01 d0                   add    %edx,%eax
 8048e83:   89 43 04                mov    %eax,0x4(%ebx)
 8048e86:   40                      inc    %eax
 8048e87:   50                      push   %eax
 8048e88:   ff 33                   pushl  (%ebx)
 8048e8a:   89 55 e4                mov    %edx,-0x1c(%ebp)
 8048e8d:   e8 4a fb ff ff          call   80489dc <realloc@plt>
 8048e92:   83 c4 10                add    $0x10,%esp
 8048e95:   85 c0                   test   %eax,%eax
 8048e97:   8b 55 e4                mov    -0x1c(%ebp),%edx
 8048e9a:   74 0f                   je     8048eab <_writeDataToBuffer+0x43>
 8048e9c:   89 03                   mov    %eax,(%ebx)
 8048e9e:   8b 4b 04                mov    0x4(%ebx),%ecx
 8048ea1:   c6 04 08 00             movb   $0x0,(%eax,%ecx,1)
 8048ea5:   85 f6                   test   %esi,%esi
 8048ea7:   75 1b                   jne    8048ec4 <_writeDataToBuffer+0x5c>
 8048ea9:   eb 27                   jmp    8048ed2 <_writeDataToBuffer+0x6a>
 8048eab:   83 ec 0c                sub    $0xc,%esp
 8048eae:   68 c0 92 04 08          push   $0x80492c0
 8048eb3:   e8 44 fc ff ff          call   8048afc <puts@plt>
 8048eb8:   c7 04 24 01 00 00 00    movl   $0x1,(%esp)
 8048ebf:   e8 78 fc ff ff          call   8048b3c <exit@plt>
 8048ec4:   8b 03                   mov    (%ebx),%eax
 8048ec6:   03 43 08                add    0x8(%ebx),%eax
 8048ec9:   89 c7                   mov    %eax,%edi
 8048ecb:   89 d1                   mov    %edx,%ecx
 8048ecd:   f3 a4                   rep movsb %ds:(%esi),%es:(%edi)
 8048ecf:   01 53 08                add    %edx,0x8(%ebx)
 8048ed2:   89 d0                   mov    %edx,%eax
 8048ed4:   8d 65 f4                lea    -0xc(%ebp),%esp
 8048ed7:   5b                      pop    %ebx
 8048ed8:   5e                      pop    %esi
 8048ed9:   5f                      pop    %edi
 8048eda:   5d                      pop    %ebp
 8048edb:   c3                      ret    
HalosGhost commented 10 years ago

Okay, that's perfectly valid. I will review and standardize (if necessary) the commit tomorrow.

I do wonder though. What is the point of returning a function inside void-returning functions? Why not just call the function, and be done?

mhogomchungu commented 10 years ago

_realloc() and _malloc() both return (void*) and hence they expect to return something and a compiler warning will be generated if they dont.I wrote them that way to silence a compiler warning,i could have written them as for more clarity:

void _abortProgram(void)
{   puts( "could not allocate memory,aborting" ) ;
    exit(1);
}
void * _malloc(size_t size)
{  
    void * e = malloc(size);
    if ( e ){
        return e;
    }else{
        _abortProgram();
                /*
                  This part will never be reached and its here just to silence a compiler
                   warning since the function expects a value to be returned.
               */
        return NULL
    }
}