3

I have a function for writing ppm files (a picture format) to disk. It takes the filename as a char* array. In my main function, I put together a filename using a stringstream and the << operator. Then, I want to pass the results of this to my ppm function. I've seen this discussed elsewhere, often with very convoluted looking methods (many in-between conversion steps).

What I've done is shown in the code below, and the tricky part that others usually do in many steps with temp variables is (char*) (PPM_file_name.str().data()). What this accomplishes is to extract the string from stringstream PPM_file_name with .str(), then get the pointer to its actual contents with .data() (this is a const char*), then cast that to a regular (char*). More complete example below.

I've found the following to work fine so far, but it makes me uneasy because usually when other people have done something in a seemingly more convoluted way, it's because that's a safer way to do it. So, can anyone tell me if what I'm doing here is safe and also how portable is it?

Thanks.

#include <iostream>
#include <sstream>
#include <stdio.h>
#include <string>
using namespace std;

int main(int argc, char *argv[]){

    // String stream to hold the file name so I can create it from a series of other variable
    stringstream PPM_file_name; 

    // ... a bunch of other code where int ccd_num and string cur_id_str are created and initialized

    // Assemble the file name
    PPM_file_name << "ccd" << ccd_num << "_" << cur_id_str << ".ppm";

    // From PPM_file_name, extract its string, then the const char* pointer to that string's data, then cast that to char*
    write_ppm((char*)(PPM_file_name.str().data()),"ladybug_vidcapture.cpp",rgb_images[ccd_num],width,height);                   

    return 0;
}
skaffman
  • 398,947
  • 96
  • 818
  • 769
SSilk
  • 2,433
  • 7
  • 29
  • 44
  • Are you actually changing the filename in write_ppm? –  Nov 11 '10 at 16:43
  • 2
    I'd suggest using `const_cast` instead of `(char*)` here, just to be clear on your intent. – Nathan Ernst Nov 11 '10 at 16:48
  • 1
    If you're dealing with strings, you should use c_str() instead of data() to access the std::string object. c_str() appends a null character, data() does not. – badgerr Nov 11 '10 at 16:53

6 Answers6

3

Thanks everyone. So, following a few peoples' suggestions here, I've done the following, since I do have control over write_ppm:

Modified write_ppm to take const char*:

void write_ppm(const char *file_name, char *comment, unsigned char *image,int width,int height)

And now I'm passing ppm_file_name as follows:

write_ppm((PPM_file_name.str().c_str()),"A comment",rgb_images[ccd_num],width,height);

Is there anything I should do here, or does that mostly clear up the issues with how this was being passed before? Should all the other char arguments to write_ppm be const as well? It's a very short function, and it doesn't appear to modify any of the arguments. Thanks.

SSilk
  • 2,433
  • 7
  • 29
  • 44
  • This is the idiomatic way to solve the problem - if the function is taking a string that won't be modified, declare it as `const char *`. Then it will accept a string literal or a `c_str()` interchangeably. – Mark Ransom Nov 15 '10 at 06:17
2

This looks like a typical case of someone not writing const-correct code and it having the knock-on effect. You have several choices:

  • If write_ppm is under your control, or the control of anyone you know, get them to make it const corrct

  • If it is not, and you can guarantee it never changes the filename then const_cast

  • If you cannot guarantee that, copy your string into a std::vector plus the null terminator and pass &vec[0] (where vec represents the name of your vector variable)

CashCow
  • 30,981
  • 5
  • 61
  • 92
1

  1. You should use PPM_file_name.str().c_str(), since data() isn't guaranteed to return a null-terminated string.

  2. Either write_ppm() should take its first argument by const char* (promising not to change the string's content) or you must not pass a string stream (because you must not change its content that way).

You shouldn't use C-style casts in C++, because they don't differentiate between different reasons to cast. Yours is casting away const, which, if at all, should be done using const_cast<>. But as a rule of thumb, const_cast<> is usually only required to make code compile that isn't const-correct, which I'd consider an error.

sbi
  • 219,715
  • 46
  • 258
  • 445
1

It's absolutely safe and portable as long as write_ppm doesn't actually change the argument, in which case it is undefined behavior. I would recommend using const_cast<char*> instead of C-style cast. Also consider using c_str() member instead of the data() member. The former guarantees to return a null-terminated string

Armen Tsirunyan
  • 130,161
  • 59
  • 324
  • 434
0

Use c_str() instead of data() (c_str() return a NULL-terminated sequence of characters).

Armen Tsirunyan
  • 130,161
  • 59
  • 324
  • 434
Luis G. Costantini R.
  • 1,028
  • 7
  • 6
0

Why not simply use const_cast<char *>(PPM_file_name.str().c_str()) ?

Marcin
  • 12,245
  • 9
  • 42
  • 49