Jump to content

  • Log In with Google      Sign In   
  • Create Account


bytes data = read_file_and_alloc_bytes("data.txt");


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
8 replies to this topic

#1 fir   Members   -  Reputation: -452

Like
0Likes
Like

Posted 31 December 2013 - 03:57 AM

I wonder if such style of coding is okay in c and how to do that,

 

let me explain I want load disk file into ram with just one function

call of my production

 

I defined

 

struct bytes

{

  char* begin;

  char* end;  //end is a first char after the data

}

 

the function mallocs appriopriate ram ant then reads whole file into

it and thats all

 

 int get_file_size(FILE* file)
 {
  int seek_pos = ftell(file);
  fseek(file, 0, SEEK_END);
  int file_length = ftell(file) ;
  fseek(file, seek_pos, SEEK_SET);
 
  return file_length;
 }
 
 bytes read_file_and_alloc_bytes(char* file_name)
 {
  FILE * file = fopen(file_name, "rb");
   if(file == NULL)      ERROR_EXIT("\n input file %s not found", file_name);
 
  int file_length = get_file_size(file);
  char* data      = malloc(file_length);
 
  if(data == NULL)   ERROR_EXiT("\n could not malloc data for file");
 
  int elements_read = fread(data, 1, file_length, file);
  int error = fclose(file);
 
  if(error)  ERROR_EXIT("error closing file %s", file_name);
 
  bytes loaded_bytes = {data, data + file_length};
 
  return loaded_bytes;
}
ERROR_EXIT() is just a function wrapper on printf and also cals exit(1)
 
the questions:
 
1) is this genrerally correct (reasonable way of coding)?
2) what with names? specificaly is the name bytes for such chunk of ram ok
3) can it be in some way improved?
 


Sponsor:

#2 richardurich   Members   -  Reputation: 1187

Like
0Likes
Like

Posted 31 December 2013 - 04:28 AM

I'd pass back the data and size rather than a begin and end pointer. This makes it clear you didn't allocate memory for end. And data size is usually more useful than an end pointer anyways.

 

Bytes isn't the best name, but it's not the worst either. Seeing bytes as a data type, I would incorrectly assume it's just a byte[] or such at first glance.

 

You don't check that fread actually read the bytes, so you could improve your code by making sure fread actually succeeded.



#3 fir   Members   -  Reputation: -452

Like
0Likes
Like

Posted 31 December 2013 - 06:05 AM

I'd pass back the data and size rather than a begin and end pointer. This makes it clear you didn't allocate memory for end. And data size is usually more useful than an end pointer anyways.

 

Bytes isn't the best name, but it's not the worst either. Seeing bytes as a data type, I would incorrectly assume it's just a byte[] or such at first glance.

 

You don't check that fread actually read the bytes, so you could improve your code by making sure fread actually succeeded.

 

as to size instead of end i am not sure, also as to name bytes

i am not sure too, it is about some chunk of ram, hard to decide what name to use - it should be common name good for common acceptance becouse this is some low level based type

 

as to fread tnx i moslooked that

 

i am also not sure if i could give three parts info error or just only one

 

for example 

 

  if(file == NULL)     ERROR_EXIT("\n cannot open file %s \n fopen(\"%s\", \"rb\") returns NULL \n  file: %s  line %d ", file_name, file_name, __FILE__, __LINE__);
 
gives the output
 
 ERROR:
 cannot open file input.txt
 fopen("input.txt", "rb") returns NULL
  file: main.c  line 60
 
 program exit
 
not sure if i should give the failed function call line and also
code location info as an output..
 


#4 Bregma   Crossbones+   -  Reputation: 5013

Like
0Likes
Like

Posted 31 December 2013 - 06:23 AM

if(file == NULL) ERROR_EXIT("%s(%d) cannot open file '%s': %s\n", __FUNCTION__, __LINE__, file_name, strerror(errno));

 

That gives pretty much all the information you need.  Giving the source line and the args to the call and the fact that it's indicated failure when there's a failure is useless redundancy, but the reason for the failure is the most useful thing and you're missing it.

 

Note I also put quotes around the filename so it's obvious it's empty (or contains nonprintable characters).  You'll need to include <string.h> and <errno.h> to get the appropriate declarations for strerror() and errno.


Stephen M. Webb
Professional Free Software Developer

#5 fir   Members   -  Reputation: -452

Like
0Likes
Like

Posted 31 December 2013 - 06:30 AM

 

if(file == NULL) ERROR_EXIT("%s(%d) cannot open file '%s': %s\n", __FUNCTION__, __LINE__, file_name, strerror(errno));

 

That gives pretty much all the information you need.  Giving the source line and the args to the call and the fact that it's indicated failure when there's a failure is useless redundancy, but the reason for the failure is the most useful thing and you're missing it.

 

Note I also put quotes around the filename so it's obvious it's empty (or contains nonprintable characters).  You'll need to include <string.h> and <errno.h> to get the appropriate declarations for strerror() and errno.

 

 

but i would like to use one 'global' ERROR_EXIT(...) function for all my erroring purposes (called after all whole program failed function fails and other wrong conditions detected ) - the "strerror(errno)" would then behave correctly? what is scope of this information - only c standard lib? never used it


Edited by fir, 31 December 2013 - 06:31 AM.


#6 Bacterius   Crossbones+   -  Reputation: 8533

Like
0Likes
Like

Posted 31 December 2013 - 07:30 AM


but i would like to use one 'global' ERROR_EXIT(...) function for all my erroring purposes (called after all whole program failed function fails and other wrong conditions detected ) - the "strerror(errno)" would then behave correctly? what is scope of this information - only c standard lib? never used it

 

The macro is declared in a header somewhere, put the required #include's in it too. They will be pulled into every unit that happens to use the macro, which is precisely when they will be needed.

 

Note I would only use this for debugging purposes during development. It is better style (but not the only possibility, of course) to use a more structured form of error handling where errors are handled by the appropriate function along the call stack (eventually bubbling up to the user in the form of a sane error message the user can understand with optional guru meditation, though it can be handled at a lower level if a given function expects an error it can react to in a safe way) instead of crashing the program instantly. For library code it is completely unacceptable to crash the host program.


The slowsort algorithm is a perfect illustration of the multiply and surrender paradigm, which is perhaps the single most important paradigm in the development of reluctant algorithms. The basic multiply and surrender strategy consists in replacing the problem at hand by two or more subproblems, each slightly simpler than the original, and continue multiplying subproblems and subsubproblems recursively in this fashion as long as possible. At some point the subproblems will all become so simple that their solution can no longer be postponed, and we will have to surrender. Experience shows that, in most cases, by the time this point is reached the total work will be substantially higher than what could have been wasted by a more direct approach.

 

- Pessimal Algorithms and Simplexity Analysis


#7 nfactorial   Members   -  Reputation: 729

Like
0Likes
Like

Posted 01 January 2014 - 10:38 AM

I would change it slightly, my main issue with the original is that the function call leaves it up-to the caller to free the memory and the allocation is also hard-inlined within the call. Thus there's no way of changing the allocation and the caller may expect the memory to be allocated in a different way and thus free it incorrectly. For example, if the caller tries to release the memory with delete rather than free, the function as is just expects the caller to know what to do. I realise it's a personal project, but 6 months down the line you may be the one who forgets :)

 

I would change it so the function is passed either an allocator or a buffer object. for example:

 

(pseudo-code)

int read_file_and_alloc_bytes( MemoryBuffer &buffer, const char *fileName )
{
    buffer.Allocate( file_size );
    return read( buffer.rawBuffer, file_size );
}
 

Which means allocation and release are now managed by the MemoryBuffer object, it also allows the memory buffer to have different implementations with different memory behaviour etc.

 

n!


Edited by nfactorial, 01 January 2014 - 10:40 AM.


#8 fir   Members   -  Reputation: -452

Like
0Likes
Like

Posted 01 January 2014 - 12:42 PM

I would change it slightly, my main issue with the original is that the function call leaves it up-to the caller to free the memory and the allocation is also hard-inlined within the call. Thus there's no way of changing the allocation and the caller may expect the memory to be allocated in a different way and thus free it incorrectly. For example, if the caller tries to release the memory with delete rather than free, the function as is just expects the caller to know what to do. I realise it's a personal project, but 6 months down the line you may be the one who forgets smile.png

 

I would change it so the function is passed either an allocator or a buffer object. for example:

 

(pseudo-code)

int read_file_and_alloc_bytes( MemoryBuffer &buffer, const char *fileName )
{
    buffer.Allocate( file_size );
    return read( buffer.rawBuffer, file_size );
}
 

Which means allocation and release are now managed by the MemoryBuffer object, it also allows the memory buffer to have different implementations with different memory behaviour etc.

 

n!

somewhat good point but this is then some digression because

it is not c ... if so it seems that you can do it simpler by

 

buffer.read_file("filename.txt");
buffer.free();

 

in c i got two separate functions instead



#9 Paradigm Shifter   Crossbones+   -  Reputation: 5256

Like
0Likes
Like

Posted 01 January 2014 - 01:02 PM

Either pass a user allocated buffer (so they allocate and free it) or have 2 functions: 1 to alloc and read and another to free the memory returned from the alloc and read function. It doesn't matter if the free function just calls free on the buffer it should still be a separate function so if you do change the allocation implementation you only need to change the freeing function.


"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS