best practice on RAII file loading

[jiggunjer]

I have a program I that looks like C-style programming and want to re-write it using STL stuff like vectors or strings. The function in question is a file loader. There is an array of chars being made, this is referenced by two pointers a char* and a cast unsigned char*. This made me think: is it possible to have two std::vectors/std::arrays share the same data?

On another note, how to go about loading (std::fread) the file without using `new[]`, keeping in mind how the data will used later. I found a vector<char> not good for string comparisons, maybe a string s(2048)?

      std::FILE* nfile = fopen(filename,"rb");
      char *const header = new char[2048]; 
      *header = 0; //dunno why they initialize first char...
      const unsigned char *const uheader = (unsigned char*)header;
      const unsigned int siz = (unsigned int)std::fread(header,2048,1,nfile);   // Read first 2048 bytes. 

//...code goes on to compare the header and uheader with string literals and hexadecimal numbers respectively

just using 2 vectors for char and unsigned char I'd end up with 2 copies of the header data.

[Dave11]

my initial thinking: why not use std::string as buffer + std:: ifstream ?

std::string buffer (2048,0);
std::ifstream fileReader (nfile/*,std::ifstream::binary*/);
if (fileReader.is_open() && fileReader.good()){
   fileReader.read(&buffer[0],2048);
}

//do somthing with buffer

[algorism]

The code snippet you showed overuses const. In fact, only the leftmost const on uheader is very useful.

*header = 0 is treating the buffer as a c-style (zero-terminated) string, which it is not.

It's not always appropriate to put data into a vector (do you need the resizing capability?). A std::array might be better.

constexpr size_t BufSize = 2048;
std::ifstream fin(filename, std::ios::binary);
std::array<char, BufSize> aheader;
if (!fin.read(aheader.data(), BufSize))
    ; // read error (eof?)
size_t siz = fin.gcount();
char *header = aheader.data();
const unsigned char *uheader = static_cast<unsigned char *>(header);

On the other hand, the existence of uheader is a little fishy and it may not be needed at all.
Why not just cast as needed?

[jiggunjer]

@algorism
Shouldn't the *header be '\0' instead of 0? and why--the array gets filled up by fread() 2 lines down...
I considered std::array too (completely forgot about data()! ), after loading I can just make header a const char* (instead of char*) and use it with std::strncmp later.

If the code doesn't use uheader it would need to cast alot, e.g.

else if (uheader[0]==0x89 && uheader[1]==0x50 && uheader[2]==0x4E && uheader[3]==0x47 &&  // PNG.
           uheader[4]==0x0D && uheader[5]==0x0A && uheader[6]==0x1A && uheader[7]==0x0A) f_type = _png;

there are a few if statements like the one above. I can't imagine putting (unsigned char*) in front of every header[] call. I was also considering uint8_t to replace unsigned char.

@Dave11
that was my first thought too. but the difference between fread() and an ifstream seems negligible, and a string is cumbersome to use for unsigned char work.

[algorism]

@algorism
Shouldn't the *header be '\0' instead of 0? and why--the array gets filled up by fread() 2 lines down...
I considered std::array too (completely forgot about data()! ), after loading I can just make header a const char* (instead of char*) and use it with std::strncmp later.

0 will get converted to '\0' in that context. (In C I believe they are identical. char constants in C are ints, unless that's changed recently.)

In this context there's no benefit in setting the first element to 0.


You seem to (possibly) be under the impression that strncmp (and similar) need to be passed a const char *. That's not true. It's perfectly okay to pass them a char *. They just promise not to change the data. The rule is that you can pass a char * to a const char * interface but not a const char * to a char * interface (since the char *interface doesn't promise not to change it).

If the code doesn't use uheader it would need to cast alot, e.g.

else if (uheader[0]==0x89 && uheader[1]==0x50 && uheader[2]==0x4E && uheader[3]==0x47 &&  // PNG.
           uheader[4]==0x0D && uheader[5]==0x0A && uheader[6]==0x1A && uheader[7]==0x0A) f_type = _png;

there are a few if statements like the one above. I can't imagine putting (unsigned char*) in front of every header[] call. I was also considering uint8_t to replace unsigned char.

Overall you'd do best to pick a type. Why can't you just use unsigned char in the first place?

[jiggunjer]

yea I just checked strncmp, because I wasn't sure either why I thought that--maybe I'm seeing to many consts everywhere.
I could use unsigned char for loading (aheader). But then I'd need to compare them to string literals elsewhere in the code, e.g.

if(!std::strncmp(header,"OFF\n",4)) file_type = _off;

would need casting to char.

I suppose I could manually translate the string literals into hex values and compare them all uint8_t... I don't like comparing characters, but I don't want to use the convenient std::string because it makes copies.

[Elysia]

...but I don't want to use the convenient std::string because it makes copies.

Wut? Did you TEST the code to make sure it matters?
With move semantics, I don't think it should have that much overhead. At all.

[algorism]

yea I just checked strncmp, because I wasn't sure either why I thought that--maybe I'm seeing to many consts everywhere.
I could use unsigned char for loading (aheader). But then I'd need to compare them to string literals elsewhere in the code, e.g.

if(!std::strncmp(header,"OFF\n",4)) file_type = _off;

would need casting to char.

I suppose I could manually translate the string literals into hex values and compare them all uint8_t... I don't like comparing characters, but I don't want to use the convenient std::string because it makes copies.

I don't see how std::string is any better in this context.

Maybe you could wrap a std::array in a class that adds conversion operators to char* and unsigned char*.

Or you could use the mem* functions instead of the str* functions since you aren't dealing with zero-terminated strings. Look at memcmp. They won't complain about pointer types.