#20 fix CString::LoadString behavior for string lists

[risa2000]

When string in resource is defined with inner '\0'
character (as in string lists). LoadString resource
omits all characters after the first '\0'. For string
larger than 256 chars, this is not true however.
This patch fixes that.

[risa2000]

patch to CString::LoadString

[Igor Tandetnik]

Logged In: YES
user_id=964802

I've just tested it - the problem is there both for short
(shorter than 256 characters) and long strings. The fix is
pretty easy for both cases:

// *this = szTemp;
AssignCopy(nLen, szTemp); // as in your patch

// ReleaseBuffer();
ReleaseBuffer(nLen);

I have two concerns that stop me from just committing the
fix:

1. Is there a chance for this change to break existing code?
WTL::CString is supposed to mimic MFC's CString, to ease
the porting of existing MFC code. MFC's CString::LoadString
has the same problem, and there might be code lying around
that actually relies on this behavior. Unlikely IMHO, and
besides, CStringT in VC7[.1] honors embedded NULs in
LoadString, so any code relying on buggy behavior will have
to be fixed sooner or later anyway.

2. Support for embedded NULs in CString is spotty at best.
Even the copy constructor may lose data after a NUL in some
cases, and preserve it in others. You can have s1 == s2 but
s1.GetLength() != s2.GetLength(). And so on.
Trying to implement a consistent support for embedded NULs
would open a huge can of worms, and will probably break
existing code. I doubt it is wise to do so now. So my concern
is, what's the rationale for adding such support in some
places such as LoadString, if we don't add it thoughout?

[risa2000]

Logged In: YES
user_id=603791

I did not tested strings longer than 256 chars, just assumed
the behavior from the code, so it is good, you spotted
another problem. To your comments:

ad 1) I do not see any reason for trying to mimick (buggy)
behavior in different library (MFC), so I am for fix :).

ad 2) The reason for support is simple. I have resource
string formatted as string list and thought it might be
easier to use CString::LoadString the same way as it is used
on ordinary strings. Otherwise I would need to reimplement
basically the same functionality LoadString already provides
just for this special case. So the question is more like: Is
it better to have method for uniform access to string
resources, or is it better to preserve current (rather)
uniform CString string list misinterpretation?

Side Note: There is also "buggy" behavior in ::LoadString,
which omits any trailing '\0', so "A\0B\0\C\0\0" in resource
file is loaded as is "A\0B\0\C\0" instead, putting three
trailing '\0' there does not help either.

I do not know what are other limitatitons of CString (you
mentioned as "spotty", I did not checked the code) when
dealing with string list, but if I take comparison to for
example STL I would expect that correct behavior should
honor the size of the current buffer unless it explicitly
deals with const TCHAR * (LPCTSTR). So I would expect to
loose string list in operations of explicit intialization,
in any other CString operation, it should be preserved.
Well, at least in theory :).

Or would you suggest using CStringT instead? Since you
mentioned it, I would like to ask if you have any experience
with this. I was considering using it, but was not sure, if
I want additional dependency on mfc DLL (which is, if I
understood it properly, inevitable).

[Igor Tandetnik]

Logged In: YES
user_id=964802

> I have resource string formatted as string list and thought
it might be easier to use CString::LoadString the same way
as it is used on ordinary strings.

The problem is that you must be extremely careful when
working with such a CString instance. Very few methods of
CString take the possibility of embedded NULs into account,
so most of your natural expectations are off. You are walking
over a minefield and must be extremely careful. Essentially,
you have to consult the source code for every operation you
might want to perform on CString, to check how it would
behave in the presence of embedded NULs. So my concern
is, should we really make it any easier to get into this
situation in the first place?

> Side Note: There is also "buggy" behavior in ::LoadString,
which omits any trailing '\0'

It seems that resource compiler strips trailing NULs, so they
never make it into the executable in the first place. Try
accessing the raw string resource as shown in KB200893,
and you'll see that trailing NULs are not there.

> I do not know what are other limitatitons of CString

Lots. operator==, Find, SpanIncluding and so on - all work
only up to the first NUL.

> If I take comparison to for example STL I would expect
that correct behavior should honor the size of the current
buffer unless it explicitly deals with const TCHAR *
(LPCTSTR).

It sounds reasonable to me, too - in theory. In practice, it
would require a massive rewrite of CString, complete with
code bloat and possibly breaking existing code. You cannot
use existing CRT functions and would have to write your own
instead. E.g., operator== now uses strcmp, Find uses strstr -
none of them work with embedded NULs.

Since WTL::CString is essentially just a stop-gap measure
for VC6, I doubt such a massive rewrite is reasonable at this
time.

> Would you suggest using CStringT instead?

If you use VC7[.1], I would recommend doing exactly this.
Note that LoadString is "fixed" there, but other methods still
behave unreliably in the presence of embedded NULs.

> was not sure, if I want additional dependency on mfc DLL
(which is, if I understood it properly, inevitable).

This is a myth. As of VC7, CString is completely separated
from MFC. It is a template class with all its code in header
files - just like std::string - and it does not require any
additional libraries. It even works in MinCRT builds.

[risa2000]

Logged In: YES
user_id=603791

You are right. I have already checked CStringT again, and
found out that indeed it is template class :). So, the worry
about dependancy was just misunderstanding the MSDN which says:
------------------
If using in an ATL application:

CString, CStringA, and CStringW are exported from the MFC
DLL (MFC7x.DLL), never from user DLLs. This is done to
prevent CStringT from being multiply defined.
------------------
I do not what they meant with this, but for my setup, there
is no dependancy in compiled code. So I started to use
CStringT instead and therefore I am no longer depending on
changes I suggested in patch.

Concerning the resource, I have too verified that the
trailing '\0' are removed by rc, so there is no way to
resurrect them in from the resources. I might have used
RCDATA, but this does not work with CString::LoadString so,
this is not much better.

Concerning the CString class, one thing comes to my mind,
which I am missing. FormatMessage from SYSTEM, or MODULE.
There are only from STRING variant. This is however not big
deal to call ::FormatMessage, so I do not complain.

So the conclusion is, I have no objection to either way you
decide to take. Since the CStringT seems probably better
maintained then its WTL counterpart I do not see the need to
fix it. Maybe, it could be marked as "deprecated" in favor
of CStringT.

Anyway, thanks for the attention and feedback and btw, I am
using VS 7.1.

[Igor Tandetnik]

Logged In: YES
user_id=964802

OK then, closing as "won't fix".