C++ Simple IF clause changes value of a static const char variable

Question

Okay, I've been dealing with this for two days now, and I can't find a solution.

Problem: I'm trying to set a filter to a File Selection Dialog using Winapi. I'm using GetOpenFileName function to do this. This function uses a structure to set options such as file extension filters. This structure's member called lpstrFilter needs a certain string format. I'm setting that string exactly as Winapi indicates, but for some reason, this string's value changes.

I've got this static const char *:

//This contains string "JPG"
static const char * extensionFilter = v->trabajo10.C_JMV_SelectFile_FileExtension7.GetString();

//This forms a filter string which applies to OPENFILENAME structure.
string sFilter;
sFilter.append("Format: ");
sFilter.append(extensionFilter);
sFilter.push_back('\0');
sFilter.append("*.");
sFilter.append(extensionFilter);
sFilter.push_back('\0');
const char * filter = sFilter.c_str();
ofn.lpstrFilter = filter; //This sets: --> Format: JPG\0*.JPG\0

//This opens the file selection dialog
if (GetOpenFileName(&ofn)==TRUE){
...

The File Selection Dialog looks CORRECTLY, like this:

The joke comes now, I modify the code like this:

//This contains string "JPG"
static const char * extensionFilter = v->trabajo10.C_JMV_SelectFile_FileExtension7.GetString();

if(1){
   //This forms a filter string which applies to OPENFILENAME structure.
   string sFilter;
   sFilter.append("Format: ");
   sFilter.append(extensionFilter);
   sFilter.push_back('\0');
   sFilter.append("*.");
   sFilter.append(extensionFilter);
   sFilter.push_back('\0');
   const char * filter = sFilter.c_str();
   ofn.lpstrFilter = filter; //This sets: --> Format: JPG\0*.JPG\0
}

//This opens the file selection dialog
if (GetOpenFileName(&ofn)==TRUE){
...

And this is the result, THE PROBLEM:

Filter string was modified???

Had you used a debugger, and inspected the `OPENFILENAME` structure prior to calling `GetOpenFileName`, you wouldn't have had to ask this question. This question makes up facts that aren't. — IInspectable, Dec 11 '15 at 12:52
@IInspectable I'm not able to use a debugger, the IDE I'm using won't let me. — ProtectedVoid, Dec 11 '15 at 13:37
@ProtectedVoid which IDE do you use? and you can always get a debugger separately; both WinDbg and gdb can run independent of an IDE. — andlabs, Dec 11 '15 at 13:43
@andlabs I use CA Plex 6.1. It's a quite old IDE that let's you create multi-platform applications. I have no choice, I must use this IDE. It doesn't even let you define C++ functions. It forces you to program in C++ like it was a simple scripting language. I hope you understand a bit more my limitations. — ProtectedVoid, Dec 11 '15 at 13:59
It's not actually the `if` clause. It's the curly brackets `{}` Any declaration inside the brackets fall in a different scope, they won't be seen outside the brackets. — Barmak Shemirani, Dec 12 '15 at 03:32

score 5 · Accepted Answer · edited May 23 '17 at 10:27

5

if(1){
   //This forms a filter string which applies to OPENFILENAME structure.
   string sFilter;
   sFilter.append("Format: ");
   sFilter.append(extensionFilter);
   sFilter.push_back('\0');
   sFilter.append("*.");
   sFilter.append(extensionFilter);
   sFilter.push_back('\0');
   const char * filter = sFilter.c_str();
   ofn.lpstrFilter = filter; //This sets: --> Format: JPG\0*.JPG\0
}

The sFilter variable has a lifetime that ends when the block in which it declared ends. The pointer returned by sFilter.c_str() is valid until sFilter is modified or destroyed.

You are using this pointer after it has become invalidated. This is the same problem as you had yesterday, which I guessed at in comments to the question. This is why you need to show a full MCVE. This question also looks to be a duplicate of the one that you asked a week ago: Winapi GetOpenFileName Extension Filter not working. I suggest that you take some time to make sure that you fully appreciate the validity of the value returned by c_str().

You must ensure that sFilter lives until after you have finished with the pointer. Declare sFilter in an outer block to ensure that.

edited May 23 '17 at 10:27

Community

1
1

answered Dec 11 '15 at 12:47

David Heffernan

601,492
42
1,072
1,490

I understand the problem now, thanks. But this is forcing me to create a variable in an outer block, which it means it will be defined even if I don't need it? What If I'll only use it if a condition is met? Wasn't it logical to create it when the condition is met? Thanks again. – ProtectedVoid Dec 11 '15 at 14:05
Yes, I can see why you wrote the code that way. It's an easy trap to fall in to. – David Heffernan Dec 11 '15 at 14:08
@ProtectedVoid The text is conditionally created, so it must be conditionally destroyed. And clearly it must be destroyed later than the end of the original conditional. So some object outside the original conditional must at least remember whether the text must be destroyed. Having `sFilter` declared (but default empty) outside the conditional is the simple way to accomplisg that. – JSF Dec 11 '15 at 14:14
@JSF You can clear a variable's content but the variable is declared, making your code inefficient if it's not used. What if I had 100 variables declared and they would only be used if a condition is met, all the memory allocation would be useless. I'm starting in C++ and my intention is to increase my knowledge. Thanks. – ProtectedVoid Dec 11 '15 at 14:26
If it were 100 conditional variables, it would be worth more effort to reduce the footprint of the unused case. Here that footprint could be reduced to a single `bool` telling whether `ofn.lpstrFilter` "owns" the thing it points to. That `bool` would need to be outside the conditional **and** the code inside the conditional would need to be different such that the text buffer is owned by that pointer. A default empty string is pretty low cost. So reducing the footprint of "unused" down to the minimum of one `bool` is not worth the large effort required. – JSF Dec 11 '15 at 14:32

score 2 · Answer 2 · answered Dec 11 '15 at 12:47

2

The problem is that you have a variable that fell out of scope

if(1){
   string sFilter;

   // ... code

   // Right here
   const char * filter = sFilter.c_str();      
   ofn.lpstrFilter = filter;
}

After that block ends filter falls out of scope, so ofn.lpstrFilter has a dangling pointer.

answered Dec 11 '15 at 12:47

Cory Kramer

114,268
16
167
218

Actually it makes no difference that `filter` falls out of scope. The dangling pointer is because `sFilter` fell out of scope. (I'm sure you knew that and even quoted code to make it clear. But your inexact description might confuse someone.) Moving `filter` to a wider scope wouldn't help, nor would leaving it in the narrow scope hurt. Moving just `sFilter` to a wider scope would fix the problem. – JSF Dec 11 '15 at 13:31

score 0 · Answer 3 · answered Dec 11 '15 at 14:44

Answering ProtectedVoid's concern about declaring an unused object: Imagine a std::string was an expensive object and the condition was unlikely. You don't want the object constructed unless the condition is true. But then it must last beyond the scope of the condition. So we use the fact that a default constructed unique_ptr is much cheaper than a default constructed string:

std::unique_ptr<std::string> scope_extender;
if( something unlikely ){
       //This forms a filter string which applies to OPENFILENAME structure.
       std::string* sFilter = new std::string;
       scope_extender.reset( sFilter );
       sFilter->append("Format: ");
       sFilter->append(extensionFilter);
       sFilter->push_back('\0');
       sFilter->append("*.");
       sFilter->append(extensionFilter);
       sFilter->push_back('\0');
       const char * filter = sFilter->c_str();
       ofn.lpstrFilter = filter; //This sets: --> Format: JPG\0*.JPG\0
    }

Obviously I don't want to imply std::string is expensive enough to construct to be worth all that trouble. But some object in a similar situation might be. Also, there was a comment about what if it was many minor objects in one conditional. In that case, you would want a utility struct to hold them all together and the unique_ptr conditionally owns that struct.

Thanks, nice example! I appreciate your effort. – ProtectedVoid Dec 11 '15 at 15:10 — ProtectedVoid, Dec 11 '15 at 15:10

C++ Simple IF clause changes value of a static const char variable

3 Answers3

Linked