Solved

MFC CString concatenation failure

Posted on 2008-10-14
13
2,638 Views
Last Modified: 2013-12-14
If I have a local variable of type CString and then assign to it using the concatenation operator, things work fine:

CString StringFunction1()
{
  CString one;
  one = "ONE";
  return one;
}

CString StringFunction2()
{
  CString two;
  two = "TWO";
  return two;
}

{
  CString x;
  x = StringFunction1() + StringFunction2();
  MessageBox(x);
}

However, if the second function is replaced by a member function of one of my classes,
{
  MyClass myobject;
  CString x;
  x = StringFunction1() + myobject.StringFunction();
  MessageBox(x);
}

then no concatenation happens; the messagebox just shows "ONE".

class MyClass
{
public:
  MyClass();
  CString StringFunction();
};

CString MyClass::StringFunction()
{
  return CString("CSF");
}

I've looked on various google pages, and people keep saying you can't do this because you're returning a pointer to a local variable that no longer exists. Why ? Surely "return" has something like operator= semantics, in that it constructs a copy of whatever it is, in the callER's stack space ?
0
Comment
Question by:muttley3141
  • 6
  • 5
  • 2
13 Comments
 
LVL 14

Expert Comment

by:wayside
ID: 22711111
Worked for me. I got a message box that said "ONCSF". I'm using VS2005.
#include <afxwin.h>
 

CString StringFunction1()

{

  CString one;

  one = "ONE";

  return one;

}
 

CString StringFunction2()

{

  CString two;

  two = "TWO";

  return two;

}
 
 

class MyClass

{

public:

  MyClass() {}

  CString StringFunction();

};
 

CString MyClass::StringFunction()

{

  return CString("CSF");

}
 

int main()

{

  MyClass myobject;

  CString x;

  x = StringFunction1() + myobject.StringFunction();

  MessageBox(NULL, x, x, MB_OK);

}

Open in new window

0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 22711683
It must work for VC6 as well. Please post the exact code where you got the issue. The above seems to be edited here in EE as it wouldn't compile. I assume your original code has some crudities by using same names for member variables, arguments or global variables or same names for member functions and global functions.

Note, I don't see a value in naming global functions similar or equal to member functions. But if you do so, it is a good chance to shoot yourself into the foot.

Non-static member functions should somehow refer to (non-static) member data. If not make them static functions so that they can be used without an object.  
0
 
LVL 1

Author Comment

by:muttley3141
ID: 22713169
OK. When I create a minimal program such as I posted originally, the concatenation does indeed work correctly. However, my main program still does not. It's impractical to split down the big program into a form suitable for posting here, but I'll do so if I can get it to a form which demonstrates the problem.

In my main program, I have a member function like this:
void CMC2Dlg::update_title()
{
  CString t1 = vi.GetProductName() + " - " + GetDocumentTitleBaseName();
#ifdef _DEBUG
  t1 = "(DEBUG) "+t1;
#endif
  t1 += ( (m_dstate==dirty) ? "*" : "" );
  this->SetWindowText(t1);
}

What happens is that the window text gets set to the contents of vi.GetProductName(); everything after that is lost. However, when I change the vi.GetProductName() definition to return a dummy value, it all works correctly.

So, it appears that my member function vi.GetProductName() is doing something weird with the return value.

vi is an instance of the class VersionInformation, which I created. It looks like this:

I seem to remember having a problem with const-ness in this function; I had to make something non-const even though it was const.
#include "stdafx.h"

#include "VersionInformation.h"
 

CString VersionInformation::get_string(char *buffer, const CString& s)

{

  void *pb2;

  unsigned int bs2;

  char s2buffer[300+1];

  s2buffer[0]=0x00;

  CString s2,rv;

  s2.Format("\\StringFileInfo\\%04x%04x\\%s",langu,codep,s);

  if (s2.GetLength()<sizeof(s2buffer)) 

  {

    strcpy(s2buffer,s2);

    VerQueryValue(buffer,s2buffer,&pb2,&bs2);

  }

  {

    for (unsigned int i=0;i<bs2;i++)

    {

      rv += ((char*)pb2)[i];

    }

  }

  return rv;

}
 
 

VersionInformation::VersionInformation()

{

  this->m_filedescription = "";

  this->m_productname = "";

  this->m_productversion = "";

  this->m_copyright = "";

  this->m_originalfilename = "";

  this->m_internalname = "";

  this->m_companyname = "";

  this->langu = 0;

  this->codep = 0;

}
 

VersionInformation::~VersionInformation()

{

}
 

VersionInformation& VersionInformation::operator= (const VersionInformation& vi2)

{

  m_filedescription = vi2.m_filedescription;

  m_productname = vi2.m_productname;

  m_productversion = vi2.m_productversion;

  m_copyright = vi2.m_copyright;

  m_originalfilename = vi2.m_originalfilename;

  m_internalname = vi2.m_internalname;

  m_companyname = vi2.m_companyname;

  langu = vi2.langu;

  codep = vi2.codep;

  return *this;

}
 

void VersionInformation::Load()

{

  CString s2;

  DWORD dw; // handle, apparently unused.

  void *pb2;

  unsigned int bs2;

  char *buffer = NULL;
 

  {

    char mfn[MAX_PATH];

    int rs;

    if ( GetModuleFileName(NULL,mfn,sizeof(mfn)) == 0 )  goto fail;

    mfn[MAX_PATH-1] = 0x00;

    if ( 0 == (rs=::GetFileVersionInfoSize(mfn,&dw)) ) goto fail;

    if ( NULL == (buffer=(char*)malloc(rs)) ) goto fail;

    if ( 0 == ::GetFileVersionInfo(mfn,dw,rs,buffer)) goto fail;

  }
 

  if ( 0 == VerQueryValue(buffer,"\\VarFileInfo\\Translation",&pb2,&bs2) ) goto fail;

  if ( bs2 < sizeof(DWORD) ) goto fail;

  langu = ((WORD*)pb2)[0];

  codep = ((WORD*)pb2)[1];
 

  this->m_filedescription = this->get_string(buffer,"FileDescription");

  this->m_productname = this->get_string(buffer,"ProductName");

  this->m_productversion = this->get_string(buffer,"ProductVersion");

  this->m_copyright = this->get_string(buffer,"LegalCopyright");

  this->m_originalfilename = this->get_string(buffer,"OriginalFilename");

  this->m_internalname = this->get_string(buffer,"InternalName");

  this->m_companyname = this->get_string(buffer,"CompanyName");
 

fail:

  free(buffer);

  return;

}
 
 

CString VersionInformation::GetFileDescription() { return m_filedescription; }

CString VersionInformation::GetProductName() { return CString("XXX"); /*return m_productname;*/ }

CString VersionInformation::GetProductVersion() { return m_productversion; }

CString VersionInformation::GetCopyright() { return m_copyright; }

CString VersionInformation::GetOriginalFilename() { return m_originalfilename; }

CString VersionInformation::GetInternalName() { return m_internalname; }

CString VersionInformation::GetCompanyName() { return m_companyname; }

Open in new window

0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 22713749
>>>>  t1 = "(DEBUG) "+t1;

The problem is probably here. Look at the right side of the assignment.

The left argument for the + operation is a const char* pointer. The right side is a CString which has a implicit cast operator to a const char* as well. Hence, the operator + adds some pointers and the result is a pointer as well but pointing somewhere into the (virtual) memory.  

What you need is

    t1 = CString("(DEBUG) ") + t1;

Now the left operand is a CString and the compiler knows to take the CString::operator+ instead of adding pointers.


0
 
LVL 14

Accepted Solution

by:
wayside earned 80 total points
ID: 22713779
The problem is actually an interesting property of how CString works, combined with a misunderstanding with how VerQueryValue() works.

First, VerQueryValue() - it is returning a buffer size that includes the NULL terminator, not the size that strlen() would return on the buffer - it is one more.

You have code to copy the buffer to a CString one character at a time in your get_string() function:

    for (unsigned int i=0;i<bs2;i++)
    {
      rv += ((char*)pb2)[i];
    }

The problem is that bs2 contains the size of the buffer (including the NULL), not the number of characters.

So when you get to last character, you are essentially doing

    rv += '\0';

What many people don't know is that CStrings happily work with binary data. And they keep track of their length on their own (that is, they don't use a strlen() equivalent to return the length). So when you append a NULL, you are actually embedding a NULL into your string data.

All of your other concatenatations are working correctly, but because the CString has an embedded NULL, only the part in front of the NULL gets displayed. If you look at the CString in a memory window you will see all of the characters there.

Solution:

Instead of looping through the characters and appending one by one, just assign the buffer:

    rv = (char *)pb2;

Or go through your loop one less time:

    for (unsigned int i=0; i<bs2-1; i++)  // <----  bs2 - 1
    {
      rv += ((char*)pb2)[i];
    }


Then you don't get the embedded NULL, and everything works as expected.
0
 
LVL 14

Expert Comment

by:wayside
ID: 22713835
> The left argument for the + operation is a const char* pointer. The right side is a CString which has a implicit cast operator to a const char* as well. Hence, the operator + adds some pointers and the result is a pointer as well but pointing somewhere into the (virtual) memory.  

This is incorrect, there are multiple friend operator+ functions defined that allow CString's and pointers to be used in either order.
0
What Security Threats Are You Missing?

Enhance your security with threat intelligence from the web. Get trending threat insights on hackers, exploits, and suspicious IP addresses delivered to your inbox with our free Cyber Daily.

 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 22713853
>>>> vi.GetProductName() + " - " + GetDocumentTitleBaseName();

What is the declaration of the function GetDocumentTitleBaseName?

If it returns a const char* or a LPCSTR (what is a const char* as well) then you were adding two pointers. You only can concatenate using the + operator if the left argument of any + operation is a CString.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 22713910
>>>> This is incorrect, there are multiple friend operator+ functions
I know that I added such an operator to my private string class. I don't know for sure that CString has such an operator which allows to add a left hand char pointer. I mean to remember that I recently had problems with that but I may be wrong.
0
 
LVL 14

Expert Comment

by:wayside
ID: 22713913
> What is the declaration of the function GetDocumentTitleBaseName?

Doesn't matter because vi.GetProductName() is returning a CString, you can have as many pointers to the right of the CString as you want, they are processed from left to right one at a time, with a CString as the result of each intermediate step. Walk through it in the debugger if you don't believe me.

Anyway, as I posted above the problem has nothing to do with faulty pointer arithmetic.
0
 
LVL 14

Expert Comment

by:wayside
ID: 22713953
> I don't know for sure that CString has such an operator which allows to add a left hand char pointer.

It's shown in the 6.0 CString MSDN pages, and I was able to walk though it in the debugger in VS2005 and see the functions.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 22713995
>>>>  rv += '\0';

Nice catch ;-)
0
 
LVL 1

Author Closing Comment

by:muttley3141
ID: 31505841
Thank you for pointing out my obvious error. I did know that CStrings maintained their own length and could work with binary data but, combined with my misunderstanding about VerQueryString(), my brain didn't consider it !
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 22714210
>>>> I seem to remember having a problem with
>>>> const-ness in this function; I had to make something non-const even though it was const.

I had no problem to compile after setting the member function to const.

// class definition only to make it compile

class VersionInformation

{

public:

   CString get_string(char *buffer, const CString& s) const;

private:

   DWORD langu;

   DWORD codep;

   char s[32];

};
 

CString VersionInformation::get_string(char *buffer, const CString& s) const

{

  void *pb2;

  unsigned int bs2;

  char s2buffer[300+1];

  s2buffer[0]=0x00;

  CString s2,rv;

  s2.Format("\\StringFileInfo\\%04x%04x\\%s",langu,codep,s);

  if (s2.GetLength()<sizeof(s2buffer)) 

  {

    strcpy(s2buffer,s2);

    VerQueryValue(buffer,s2buffer,&pb2,&bs2);

  }

  {

    for (unsigned int i=0;i<bs2;i++)

    {

      rv += ((char*)pb2)[i];

    }

  }

  return rv;

}

Open in new window

0

Featured Post

What Is Threat Intelligence?

Threat intelligence is often discussed, but rarely understood. Starting with a precise definition, along with clear business goals, is essential.

Join & Write a Comment

Suggested Solutions

In this article, I'll describe -- and show pictures of -- some of the significant additions that have been made available to programmers in the MFC Feature Pack for Visual C++ 2008.  These same feature are in the MFC libraries that come with Visual …
Introduction: Dynamic window placements and drawing on a form, simple usage of windows registry as a storage place for information. Continuing from the first article about sudoku.  There we have designed the application and put a lot of user int…
This tutorial covers a step-by-step guide to install VisualVM launcher in eclipse.
The viewer will learn how to use and create new code templates in NetBeans IDE 8.0 for Windows.

762 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

23 Experts available now in Live!

Get 1:1 Help Now