Compiler ignores code returning a local char* from a function

As far 开发者_如何学运维as I know the following code is bad. But, Visual Studio 2010 doesn't give me any warning.

char* CEmployee::GetEmployeeName()
{
  char* szEmployeeName = "";
  CEmployeeModel* model = GetSwitchMod();
  if (model != NULL)
  {
    szEmployeeName = model->GetName();
  }

  return szEmployeeName; 
}  

---

It's not the compiler's job to debug your code.

lint or similar static checker might find this. Try running Code Analysis if you have one of the premium VS versions that includes it. Make sure you build with /W4 and fix all warning errors.

---

You're not returning a reference to a local variable, as you're returning by value, so the local variable — the pointer — is copied.

Don't confuse the pointer with its pointee.

If anything, you'd be returning a dangling pointer (though in practice the string literal buffer is likely to be in static memory somewhere). Dangling pointers don't tend to be diagnosed at compile-time.

If model->GetName() returns a dynamically-allocated buffer, making the pointer no longer point to the string literal, then your code is fine.

TRWTF is that you didn't write char const* szEmployeeName = "". Leaving out the const has been deprecated for over a decade, and is illegal in C++0x. It's a concern that so many people are still doing this.

It's even worse that there are still people using char* for strings, instead of std::string.

---

Returning szEmployeeName here is actually not an error - the string is allocated statically in read-only memory (the .rodata section in ELF executables). Quoting the (C++03) Standard:

2.13.4.1

An ordinary string literal has type “array of n const char” and static storage duration (3.7), where n is the size of the string as defined below, and is initialized with the given characters.

3.7.1

All objects which neither have dynamic storage duration nor are local have static storage duration. The storage for these objects shall last for the duration of the program

On the other hand, trying to modify this string results in undefined behaviour - in this particular case, you'll most likely get a crash at runtime. szEmployeeName should be really declared as const char* (and there are historical reasons why the standard allows initializing a plain char * with a string literal). Again, quoting the Standard:

2.13.14.2

The effect of attempting to modify a string literal is undefined.

---

You're returning a pointer to a char at the end. Are you sure the memory that the pointer is referring to is still active when the code leaves the function* (what is the lifetime of model->GetName()'s return)

*EDIT: "loop" is wrong.

---

This code isn't necessarily "wrong" in all cases. If the thing pointed to by the pointer returned from GetName is still alive, and the pointer returned from GetEmployeeName is not written to then the code appears to be well-formed. The compiler can't reasonably be expected to do a full analysis of all your code to tell you if there's an actual problem with your pointer manipulation.

You should be using std::string as @Tomalak Geret'kal noted in his answer. That then resolves all these lifetime issues.

There's a certain point at which you should be able to say "Why am I writing code this way???" and the compiler isn't going to go to extra-ordinary lengths to warn you about every possible undefined behavior in your program (it's undefined for a reason).

---

This code is fine. There's nothing going on here that could possibly cause the target of szEmployeeName to be freed.

If model is NULL, then you return a pointer to "". Using a non-const pointer certainly is questionable, but the string literal "" survives for the lifetime of your program, it's not an error to return it.

If model is non-null, you return the pointer returned by model->GetName(). Since CEmployee::GetEmployeeName() doesn't free any memory, the pointer is just as valid when returned as it was when you got it from model->GetName(). Specifically, either the pointer is valid, or it is a dangling pointer, indicating a bug in CEmployeeModel->GetName().

There are no circumstances where CEmployeeModel::GetName() is correct but CEmployee::GetEmployeeName returns a bad pointer.