StringCchCopy question
in [MFC]
|
Prev: stack overflow problem
Next: Unicode compile question
From: RB on 25 Apr 2010 11:15 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"); 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 }
From: Giovanni Dicanio on 25 Apr 2010 13:00 "RB" <NoMail(a)NoSpam> ha scritto nel messaggio news:#Rf10oI5KHA.5880(a)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
From: Oliver Regenfelder on 25 Apr 2010 13:07 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. > 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
From: Joseph M. Newcomer on 25 Apr 2010 14:25 See below... On Sun, 25 Apr 2010 11:15:43 -0400, "RB" <NoMail(a)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: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm
From: Joseph M. Newcomer on 25 Apr 2010 14:37
See below... On Sun, 25 Apr 2010 19:07:34 +0200, Oliver Regenfelder <oliver.regenfelder(a)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 Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm |