StringCchCopy question
RB
involved with using the old string runtime routines like strcpy etc.
However I am back again after a short hiatus, trying to implement said routines
and due to my novice programming ability I need advice and criticism on the best
way to do this.
I have the following downloaded example from some webpage I found on a search
CHAR szRem[32];
hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
which of course seems ok to me ( for all I know ) and I could implement it easy enough.
---------------
But my curiousity probes me to wonder why the below code I wrote and got to compile
with no errors would not work as well. Of more pertinantly please tell me where and why
my code is wrong, unsafe or all of the above and what would be the "best" way to
implement this.
LOGFONT NewFontLogStruct;
if ( StringCchCopy( ( TCHAR* ) &NewFontLogStruct.lfFaceName, sizeof ( NewFontLogStruct.lfFaceName), "Courier New" ) == S_OK);
{ //rest of program execution if the above StringCchCopy went ok }
Giovanni Dicanio
news:#Rf10oI5...@TK2MSFTNGP04.phx.gbl...
> I have the following downloaded example from some webpage I found on a
> search
>
> CHAR szRem[32];
> hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
>
> which of course seems ok to me ( for all I know ) and I could implement it
> easy enough.
I don't like above code:
I think there is an incoherence, because CHAR is used in defining 'szRem'
variable, but there is a scale by /sizeof(TCHAR) in StringCchCopy call.
Moreover, the string literal "PM_REMOVE" is undecorated without _T/TEXT
macros.
So, the above code would run just fine in ANSI builds.
I would rewrite it using TCHAR's like this:
TCHAR szRem[32];
hResult = StringCchCopy( szRem, _countof(szRem), _T("PM_REMOVE"));
This should compile in both ANSI and Unicode builds.
_countof() macro returns the count of TCHAR's in szRem static array.
_T("...") decoration expands to "PM_REMOVE" (ANSI string) in ANSI builds,
and to L"PM_REMOVE" (Unicode string) in Unicode builds.
If you want to use sizeof() instead of _countof, you should use
StringCbCopy:
"StringCbCopy Function"
http://msdn.microsoft.com/en-us/library/ms647499(VS.85).aspx
sizeof() returns the count of bytes in the static array, instead _countof()
returns the count of TCHAR's.
sizeof() == _countof() only in ANSI builds, but not in Unicode builds (in
fact, sizeof(WCHAR) == 2 [bytes]).
> ---------------
> But my curiousity probes me to wonder why the below code I wrote and got
> to compile
> with no errors would not work as well. Of more pertinantly please tell me
> where and why
> my code is wrong, unsafe or all of the above and what would be the "best"
> way to
> implement this.
>
> LOGFONT NewFontLogStruct;
> if ( StringCchCopy( ( TCHAR* ) &NewFontLogStruct.lfFaceName, sizeof (
> NewFontLogStruct.lfFaceName), "Courier New" ) == S_OK);
> { //rest of program execution if the above StringCchCopy went ok }
In above code, you don't need the TCHAR* cast and address-of (&) operator.
In fact, lfFaceName is a TCHAR static array member of LOGFONT structure:
"LOGFONT Structure"
http://msdn.microsoft.com/en-us/library/dd145037(VS.85).aspx
Moreover, based on what I previously wrote about sizeof vs. _countof and
_T() decoration, you may want to call StringCchCopy like this:
HRESULT hr = StringCchCopy(
NewFontLogStruct.lfFaceName, // dest
_countof(NewFontLogStruct.lfFaceName), // size of dest in TCHARs
_T("Courier New") ); // _T() decoration
Or use StringCbCopy with sizeof:
HRESULT hr = StringCbCopy(
NewFontLogStruct.lfFaceName, // dest
sizeof(NewFontLogStruct.lfFaceName), // size of dest in bytes
_T("Courier New") ); // _T() decoration
Moreover, I would check the return code using SUCCEEDED macro instead of
comparing directly with S_OK:
http://msdn.microsoft.com/en-us/library/ms687197(VS.85).aspx
if ( SUCCEEDED(hr) )
...
or:
if ( SUCCEEDED( StringCchCopy( ... ) )
...
HTH,
Giovanni
Oliver Regenfelder
RB wrote:
> CHAR szRem[32];
> hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
>
> which of course seems ok to me ( for all I know ) and I could implement it easy enough.
Well, the example above has several problems:
1) CHAR szREM[32]; !! Here you use the type defined as CHAR
2) hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
!! Here you use TCHAR which might be different from CHAR (or a
typo :-))
!! AND you use the size of szRem hardcoded which is REALLY bad.
you should use sizeof(szREM)/sizeof(TCHAR), or use some macro
/template which does this.
!! AND the constant string is defined as "PM_REMOVE" which
always will be a string of characters of type char, so in
case char, CHAR and TCHAR are different you are in big trouble.
better:
TCHAR szREM[32];
hResult = StringCchCopy(szRem, sizeof(szREM)/sizeof(TCHAR)
, _T("PM_REMOVE"));
this way it would be unicode aware.
> But my curiousity probes me to wonder why the below code I wrote and got to compile
> with no errors would not work as well.
> my code is wrong, unsafe or all of the above and what would be the "best" way to
> implement this.
>
> LOGFONT NewFontLogStruct;
> if ( StringCchCopy( ( TCHAR* ) &NewFontLogStruct.lfFaceName, sizeof ( NewFontLogStruct.lfFaceName), "Courier New" ) == S_OK);
You should use the following if you want to use the adress of operator:
&NewFontLogStruct.lfFaceName[0]
I presume the (TCHAR*) is your way of getting rid of the compiler
warning?
Again you need sizeof(NewFontLogStruct.lfFaceName) / sizeof(TCHAR)
and _T("Courier New") to have no unicode problems.
Best regards,
Oliver
Joseph M. Newcomer
On Sun, 25 Apr 2010 11:15:43 -0400, "RB" <NoMail@NoSpam> wrote:
>Hello a few months ago this group helped me become aware of the safety issues
>involved with using the old string runtime routines like strcpy etc.
>However I am back again after a short hiatus, trying to implement said routines
>and due to my novice programming ability I need advice and criticism on the best
>way to do this.
> I have the following downloaded example from some webpage I found on a search
>
>CHAR szRem[32];
> hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
\****
Note that you should not be dividing by sizeof(TCHAR), because you have declared the array
as CHAR. It should be
sizeof(szRem) / sizeof(szRem[0])
and this could be done in a template, which is how StringCchCopy does it. It wants a
*character* count, and you told it to copy half the characters the buffer can hold!
So it is incorrect as written. You could also do it as overloaded functions:
HRESULT StringCchCopy(CHAR * target, SIZE_T charcount, const CHAR * src);
HRESULT StringCchCopy(WCHAR * target, SIZE_T charcount, const WCHAR * src);
which would also work correctly. Note that interior to these functions, you deal with the
character-to-byte-count conversions if required.
*****
>
> which of course seems ok to me ( for all I know ) and I could implement it easy enough.
>
>---------------
>But my curiousity probes me to wonder why the below code I wrote and got to compile
>with no errors would not work as well. Of more pertinantly please tell me where and why
>my code is wrong, unsafe or all of the above and what would be the "best" way to
>implement this.
>
>LOGFONT NewFontLogStruct;
> if ( StringCchCopy( ( TCHAR* ) &NewFontLogStruct.lfFaceName, sizeof ( NewFontLogStruct.lfFaceName), "Courier New" ) == S_OK);
*****
Note that the TCHAR* cast is NOT required and could be erroneous; the
&NewFontLogStruct.lfFaceName does not need the & because the name of an array is
inherently a pointer to its type; the sizeof is incorrect because for wide characters it
gives the character count in bytes, not in characters, so it will too large by a factor of
2, and the presumption that you are using 8-bit characters for the name is erroneous; it
should be _T("Courier New")
So other than you got every parmeter completely wrong there's nothing wrong with this
code!
joe
****
> { //rest of program execution if the above StringCchCopy went ok }
>
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
Joseph M. Newcomer
On Sun, 25 Apr 2010 19:07:34 +0200, Oliver Regenfelder <oliver.re...@gmx.at> wrote:
>Hello,
>
>RB wrote:
>> CHAR szRem[32];
>> hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
>>
>> which of course seems ok to me ( for all I know ) and I could implement it easy enough.
>
>Well, the example above has several problems:
>
>1) CHAR szREM[32]; !! Here you use the type defined as CHAR
>2) hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
>!! Here you use TCHAR which might be different from CHAR (or a
> typo :-))
>!! AND you use the size of szRem hardcoded which is REALLY bad.
> you should use sizeof(szREM)/sizeof(TCHAR), or use some macro
> /template which does this.
>!! AND the constant string is defined as "PM_REMOVE" which
> always will be a string of characters of type char, so in
> case char, CHAR and TCHAR are different you are in big trouble.
>
>better:
>TCHAR szREM[32];
>hResult = StringCchCopy(szRem, sizeof(szREM)/sizeof(TCHAR)
> , _T("PM_REMOVE"));
>
>this way it would be unicode aware.
****
Actually, there is a much deeper question which I forgot to raise in my original reply:
WHY does ANYONE use a fixed-length character buffer too hold a literal string????
THe CORRECT code would be
CString Rem(_T("PM_REMOVE"));
or possibly
CStrin Rem = _T("PM_REMOVE"));
which also raises the question of why the literal is not just passed as a parameter!
It is frequently the case that beginners see a function declaration like
void DoSomething(const TCHAR * t);
and assume, for no sane reason ever understood by anyone, that they MUST have a VARIABLE
of type TCHAR * to pass in! I wonder how the education system failed to instruct them
properly in the use of a programming language; it is obvious by inspection that this can
be called as
DoSomething(_T("PM_REMOVE"));
or, if it is optional (and an empty string could be used) that it can be written as
CString Rem;
...optionally set Rem to "PM_REMOVE"
DoSomething(Rem);
One of the first rules a programmer using C++ should learn is "If you EVER write
type name[expression];
as a declaration you are probably failing to program anywhere NEAR correctly!"
This syntax is used only in exceptionally rare and exotic circumstances, and the example
using szRem here is CLEARLY not one of them! One of the rare and exotic circumstances is
when interfacing to a raw Win32 API like the LOGFONT structure, and the numerous errors in
the (mis)use of StrCchCopy have already been pointed out.
And it never ceases to amaze me how many people feel they have to copy a constant string
to a string variable to pass it into a function that takes a const argument (and therefore
will not be modifying the string) or, if it is their own function, nobody can explain why
they have failed to use 'const' for arguments that are not modified!
joe
*****
>
>> But my curiousity probes me to wonder why the below code I wrote and got to compile
>> with no errors would not work as well.
>> my code is wrong, unsafe or all of the above and what would be the "best" way to
>> implement this.
>>
>> LOGFONT NewFontLogStruct;
>> if ( StringCchCopy( ( TCHAR* ) &NewFontLogStruct.lfFaceName, sizeof ( NewFontLogStruct.lfFaceName), "Courier New" ) == S_OK);
>
>You should use the following if you want to use the adress of operator:
>&NewFontLogStruct.lfFaceName[0]
>I presume the (TCHAR*) is your way of getting rid of the compiler
>warning?
>
>Again you need sizeof(NewFontLogStruct.lfFaceName) / sizeof(TCHAR)
>and _T("Courier New") to have no unicode problems.
>
>Best regards,
>
>Oliver
DanB
>
> CHAR szRem[32];
> hResult = StringCchCopy(szRem, 32/sizeof(TCHAR), "PM_REMOVE");
Things have been said already. So in addition, if you are programing
with MFC, simply:
CString strRem( _T("PM_REMOVE") );
And get in the habit of using the _T macro. TCHAR without it doesn't
make any sense. I said it again as this will pay off later. My fingers
automatically do it any more.
> LOGFONT NewFontLogStruct;
> if ( StringCchCopy( ( TCHAR* )&NewFontLogStruct.lfFaceName, sizeof ( NewFontLogStruct.lfFaceName), "Courier New" ) == S_OK);
> { //rest of program execution if the above StringCchCopy went ok }
>
>
You should have gotten a warning about that semicolon at the end of your
if(...); Compile with warnings cranked all the way up.
You should very rarely, and only if you really know better:
#pragma warning(disable : xxxx)
The call can simply be:
StringCchCopy( lf.lfFaceName, LF_FACESIZE, _T("Courier New") );
lfFaceName will be sized according to build as there is a LOGFONTW and a
LOGFONTA.
RB
They were all excellent and very informative. And yes my (TCHAR*) was
an novice attempt to compile without an error rather than realize my code
was incorrectly implementing the address of the array[x].
All of you have given me everything I asked for and I feel that I can better
understand the concept now, which will help me implement the corrections
from a conceptual viewpoint as opposed to memorizing your examples.
Now if my Cad vocation and other home chores can allow me some time
to spend on improving my weak coding ability I can enjoy writing some
more code.
Very much appreciated, RB
RB
> declared the array as CHAR. It should be
> sizeof(szRem) / sizeof(szRem[0])
I think the above did more to move me in the right direction of ramifications of
the scale/size factor. I felt I understood this as soon as I saw it though I will have
to play with both non unicode and unicode compiles in the debugger to fully
visualize the concept.
I also read your reply to Oliver which understandably was a bit above my
understanding in whole, but I was able in part to get the direction of the context.
I have changed my implementation to the below.
if ( StringCchCopy ( NewFontLogStruct.lfFaceName,
sizeof ( NewFontLogStruct.lfFaceName) / sizeof(NewFontLogStruct.lfFaceName[0]),
_T("Courier New") ) == S_OK)
{ ..........}
------------
I know from experience that if I have still missed the mark you will let me know.
Thanks either way.
Giovanni Dicanio
news:ls19t5tepreli46eo...@4ax.com...
> It should be
>
> sizeof(szRem) / sizeof(szRem[0])
> and this could be done in a template, which is how StringCchCopy does it.
Joe: I think that StringCchCopy does not use templates.
Reading <strsafe.h> that comes with VS2010, there is the usual UNICODE
"trick":
#ifdef UNICODE
#define StringCchCopy StringCchCopyW
#else
#define StringCchCopy StringCchCopyA
#endif // !UNICODE
and the signature of StringCchCopyW is:
STRSAFEAPI
StringCchCopyW(
__out_ecount(cchDest) STRSAFE_LPWSTR pszDest,
__in size_t cchDest,
__in STRSAFE_LPCWSTR pszSrc)
(similar for StringCchCopyA).
So, there are no templates; and I think it is OK, because these functions
are exported with a pure-C interface, and templates are a C++-only concept.
Probably you had in mind that _countof() can be safely implemented using C++
templates.
In fact, for example, in VS2010's <atldef.h> header file, it can be read:
<code>
#if !defined(__cplusplus)
#define _countof(_Array) (sizeof(_Array) / sizeof(_Array[0]))
#else
extern "C++"
{
template <typename _CountofType, size_t _SizeOfArray>
char (*__countof_helper(UNALIGNED _CountofType
(&_Array)[_SizeOfArray]))[_SizeOfArray];
#define _countof(_Array) sizeof(*__countof_helper(_Array))
}
#endif
</code>
So, in case of pure C (i.e. __cplusplus not defined), _countof() is
implemented in the usual (sizeof(array)/sizeof(array[0])) way.
But in case of C++ builds, they use the power of C++ templates in defining
_countof.
Thanks to this template "magic" (template argument deduction), _countof
causes compile-time error if it is (mis)used with something different from a
static array (e.g. a pointer).
I mean, this code (which is wrong) compiles fine in C builds, but fails at
compile-time in C++ builds (VS2010):
TCHAR * pszBuffer;
int cchBuffer = _countof(pszBuffer);
Giovanni
Pete Delgado
"Joseph M. Newcomer" <newc...@flounder.com> wrote in message
news:jb29t5hvlb45496p1...@4ax.com...
> THe CORRECT code would be
>
> CString Rem(_T("PM_REMOVE"));
>
> or possibly
>
> CStrin Rem = _T("PM_REMOVE"));
Skitt's Law: "Any post correcting an error in another post will contain at
least one error itself".
Sorry Joe, I couldn't resist! The obvious typo reminded me of the top 10
internet rules! ;-) The above article is funny and interesting reading
IMHO!
-Pete
Joseph M. Newcomer
"ischemic cerebral incident" seems to have had side effects I did not expect, such as not
seeing typos and having trouble with where my left foot is going.
joe