>>> 1. please do not leave any unnecessary casting like:
>>> ( LPCTSTR ) HB_PARSTRDEF( ... )
>>> such casting does not give anything (it cannot converts strings
>>> between different formats) but can hide very serious bugs like wrong
>>> HB_PARSTRDEF() body.
>> I know about this, but if I don't cast, MSVC (C++ mode) gives me errors
>> on compile. (const WCHAR * cannot be converted to LPCWSTR or something
>> like it)
>
> It was enough to tell me about it :)
> Last night I committed modification which should resolve it:
>
> 2009-12-11 04:13 UTC+0100 Przemyslaw Czerpak (druzus/at/priv.onet.pl)
> * harbour/include/hbapicdp.h
> * define HB_WCHAR as wchar_t on Windows platforms for compilers which
> refuse to make conversions between types using the same base type.
I saw it, deleted those casts. For good, because they revealed
some problems.
>> I didn't reevaluate all of them, although I did serious cleanup
>> of parcx usage in the past. I was also thinking to fully drop it
>> for winapi calls, as I'm sure Windows won't BSOD or crash by
>> passing NULL wherever a pointer is required. That's why I didn't
>> add yet any special logic to HB_PARSTRDEF() yet. All I wanted
>> to do is to keep this information for now.
>
> Windows API is set of functions implemented on different levels
> and some of them can crash so it's necessary to check each of
> them. We cannot make any global assumption about validation of
> NULL pointers. It's even possible that the behavior is different
> in Win9x and WinXP, etc.
That's right. I remember when Mark Russinovich created a little
tools which made XP (or was it 2000?) BSOD at will, because of
unchecked parameters. MS fixed these in an SP, but older versions
I'm not so sure.
>>> If you will make mechanical translations all hb_parcx() to HB_PARSTRDEF()
>>> then I will have to also verify HB_PARSTRDEF() calls. So please try
>>> to already clean it at least for such basic things like:
>>> if( HB_ISCHAR( 1 ) )
>>> {
>>> void * hName;
>>> xec->hDLL = LoadLibrary( HB_PARSTRDEF( 1, &hName, NULL ) );
>>> hb_strfree( hName );
>>> }
>> This was just out of scope of these modification. To be honest
>> I can't easily see how above could be cleaned. In this case the
>> winapi docs doesn't mark the parameter as optional, which to me
>> means that NULL is not an accepted input. (okay it won't crash
>> if passed, so HB_PARSTRDEF() is almost surely unnecessary)
>
> In this case I wanted to show that this code checks HB_ISCHAR( 1 )
> so the parameter cannot be NULL and it's not necessary to use
> HB_PARSTRDEF() but we can simply use HB_PARSTR()
Didn't spot it, but you're right.
Anyhow we can make this revision based on PARSTRDEF usage,
that was my goal.
>>> BTW We can add to strapi.c two very simple functions:
>>> const char * hb_strnull( const char * str )
>>> {
>>> return str ? str : "";
>>> }
>>> const HB_WCHAR * hb_strnull_u16( const HB_WCHAR * str )
>>> {
>>> return str ? str : s_szConstStr;
>>> }
>>> which can be used in HB_PARSTRDEF() macro.
>> Yes, this would be the perfect solution. Anyhow I didn't
>> bother to add default value to HB_PARSTRDEF() yet, as
>> in 99% of cases the default is empty string, only one
>> was passed some other value, but it's not a requirement
>> for WAPI_*() functions to give such extra features so
>> it wasn't crucial.
>
> I think that it's necessary in code like:
> hb_retnint( ( HB_PTRDIFF ) LoadLibrary( HB_PARSTRDEF( 1, &hName, NULL ) ) );
> or at least this code should be fixed.
If not protected with HB_ISCHAR() it sure has to be.
>> If we're at this topic, I've found a few places which
>> couldn't be covered by these macros, It'd be very nice
>> if you could take a look at them and suggest the
>> cleanest way to resolve them. F.e. there are cases
>> when retrieved parameter is assigned to win structure,
>
> I do not see the problem, Can you show me the exact code place
> where it was problematic?
Here's a list of remaining HB_TCHAR* occurrences in hbwin:
---
wapi_commctrl.c:480: LPTSTR szText = HB_TCHAR_CONVTO( hb_parcx( 3 ) );
wapi_commctrl.c:488: HB_TCHAR_FREE( szText );
wapi_commctrl.c:563: LPTSTR szText = HB_TCHAR_CONVTO( hb_parcx( 3 ) );
wapi_commctrl.c:571: HB_TCHAR_FREE( szText );
wapi_commctrl.c:783: LPTSTR szText = HB_TCHAR_CONVTO( hb_parcx( 2 ) );
wapi_commctrl.c:791: HB_TCHAR_FREE( szText );
wce_simc.c:99: szAddress = HB_TCHAR_CONVFROM( PhoneEntry.lpszAddress );
wce_simc.c:100: szText = HB_TCHAR_CONVFROM( PhoneEntry.lpszText );
wce_simc.c:113: HB_TCHAR_FREE( szAddress );
wce_simc.c:114: HB_TCHAR_FREE( szText );
wce_simc.c:120: wchar_t * lpwszAddress = HB_TCHAR_CONVTO( hb_parcx( 4 ) );
wce_simc.c:121: wchar_t * lpwszText = HB_TCHAR_CONVTO( hb_parcx( 5 ) );
wce_simc.c:132: HB_TCHAR_FREE( lpwszAddress );
wce_simc.c:133: HB_TCHAR_FREE( lpwszText );
wce_smsc.c:72: wchar_t * sztMessage = HB_TCHAR_CONVTO( hb_parcx( 1 ) );
wce_smsc.c:73: wchar_t * sztPhoneNumber = HB_TCHAR_CONVTO( hb_parcx( 2 ) );
wce_smsc.c:102: HB_TCHAR_FREE( sztMessage );
wce_smsc.c:103: HB_TCHAR_FREE( sztPhoneNumber );
win_com.c:92: HB_TCHAR_CPTO( tszName, szName, sizeof( tszName ) - 1 );
win_misc.c:166: char * buffer = HB_TCHAR_CONVFROM( GetCommandLine() );
win_misc.c:204: HB_TCHAR_FREE( buffer );
win_osc.c:195: szCSDVersion = HB_TCHAR_CONVFROM( osvi.szCSDVersion );
win_osc.c:197: HB_TCHAR_FREE( szCSDVersion );
win_prn1.c:189: LPTSTR lpDocName = szDocName ? HB_TCHAR_CONVTO( szDocName
) : NULL;
win_prn1.c:198: HB_TCHAR_FREE( lpDocName );
win_prn1.c:267: LPTSTR lpData = HB_TCHAR_CONVNTO( hb_parc( 4 ), iLen );
win_prn1.c:294: HB_TCHAR_FREE( lpData );
win_prn1.c:316: lpData = HB_TCHAR_CONVNTO( hb_parc( 2 ), iLen );
win_prn1.c:325: HB_TCHAR_FREE( lpData );
win_prn1.c:377: LPTSTR lpFont = pszFont ? HB_TCHAR_CONVTO( pszFont ) :
NULL;
win_prn1.c:410: HB_TCHAR_FREE( lpFont );
win_prn1.c:448: LPTSTR lpPrinterName = pszPrinterName ? HB_TCHAR_CONVTO(
pszPrinterName ) : NULL;
win_prn1.c:487: HB_TCHAR_FREE( lpPrinterName );
win_prn1.c:498: LPTSTR lpPrinterName = pszPrinterName ? HB_TCHAR_CONVTO(
pszPrinterName ) : NULL;
win_prn1.c:525: HB_TCHAR_FREE( lpPrinterName );
win_prn1.c:598: char * pszFaceName = HB_TCHAR_CONVFROM( lplf->lfFaceName );
win_prn1.c:608: HB_TCHAR_FREE( pszFaceName );
win_prn2.c:113: char * pszData = HB_TCHAR_CONVFROM(
pPrinterEnum->pPrinterName );
win_prn2.c:115: HB_TCHAR_FREE( pszData );
win_prn2.c:160: HB_TCHAR_GETFROM( pszPrinterName, lpPrinterName,
*pnBufferSize );
win_prn2.c:175: HB_TCHAR_GETFROM( pszPrinterName, lpPrinterName,
*pnBufferSize );
win_prn2.c:222: HB_TCHAR_GETFROM( pszPrinterName,
pPrinterInfo->pPrinterName, lstrlen( pPrinterInfo->pPrinterName ) );
win_prn2.c:298: LPTSTR lpPrinterName = HB_TCHAR_CONVTO( pszPrinterName );
win_prn2.c:344: HB_TCHAR_FREE( lpPrinterName );
win_prn2.c:375: char * pszPortName = HB_TCHAR_CONVFROM(
pPrinterEnum->pPortName );
win_prn2.c:382: HB_TCHAR_FREE( pszPortName );
win_prn2.c:386: char * pszPrinterName = HB_TCHAR_CONVFROM(
pPrinterEnum->pPrinterName );
win_prn2.c:388: HB_TCHAR_FREE( pszPrinterName );
win_prn2.c:409: LPTSTR lpPrinterName = HB_TCHAR_CONVTO( pszPrinterName );
win_prn2.c:413: LPTSTR lpDocName = HB_TCHAR_CONVTO( pszDocName );
win_prn2.c:457: HB_TCHAR_FREE( lpDocName );
win_prn2.c:463: HB_TCHAR_FREE( lpPrinterName );
win_prn2.c:504: pszData = HB_TCHAR_CONVFROM(
pPrinterEnum->pPrinterName );
win_prn2.c:506: HB_TCHAR_FREE( pszData );
win_prn2.c:520: pszData = HB_TCHAR_CONVFROM(
pPrinterEnum->pPrinterName );
win_prn2.c:522: HB_TCHAR_FREE( pszData );
win_prn2.c:529: pszData = HB_TCHAR_CONVFROM(
pPrinterInfo2->pPortName );
win_prn2.c:531: HB_TCHAR_FREE( pszData );
win_prn2.c:532: pszData = HB_TCHAR_CONVFROM(
pPrinterInfo2->pDriverName );
win_prn2.c:534: HB_TCHAR_FREE( pszData );
win_prn2.c:535: pszData = HB_TCHAR_CONVFROM(
pPrinterInfo2->pShareName );
win_prn2.c:537: HB_TCHAR_FREE( pszData );
win_prn2.c:538: pszData = HB_TCHAR_CONVFROM(
pPrinterInfo2->pServerName );
win_prn2.c:540: HB_TCHAR_FREE( pszData );
win_regc.c:122: LPTSTR lpKey = HB_TCHAR_CONVTO( hb_parcx( 2 ) );
win_regc.c:180: HB_TCHAR_FREE( lpKey );
---
All of them were problematic (to me) for some reason (except WIN_CREATEFONT(),
which I'll commit ASAP.)
>> there are cases when win return value is returned
>> in array elements (win_prn),
>
> We can add hb_array[SG]etStr*() functions.
> You can also use hb_arrayGetItemPtr() and hb_itemPutStrLen*()
> but I do not like when users use hb_arrayGetItemPtr() function
> so in such case try to at least hide it by macros, i.e.:
>
> #if defined( UNICODE )
> #define HB_ARRAYSETSTR( arr, n, str ) \
> hb_itemPutStrLenU16( hb_arrayGetItemPtr( arr, n ), \
> HB_CDP_ENDIAN_NATIVE, str, lstrlenW( str ) )
> #define HB_ARRAYSETSTRLEN( arr, n, str, len ) \
> hb_itemPutStrLenU16( hb_arrayGetItemPtr( arr, n ), \
> HB_CDP_ENDIAN_NATIVE, str, len )
> #else
> #define HB_ARRAYSETSTR( arr, n, str ) \
> hb_itemPutStrLen( hb_arrayGetItemPtr( arr, n ), \
> hb_setGetOSCP(), str, strlen( str ) )
> #define HB_ARRAYSETSTRLEN( arr, n, str, len ) \
> hb_itemPutStrLen( hb_arrayGetItemPtr( arr, n ), \
> hb_setGetOSCP(), str, len )
> #endif
>
> Please only note that unlike hb_arraySetC[L]() these macros do not
> check array size and cannot be safely used with wrong indexes and
> HB_ARRAYSETSTRLEN() uses str twice so you should avoid using complex
> expressions passing it. It's a problem until we won't have added
> hb_arraySetStr*() functions. When we do that then we simply modify
> above macros to use new functions.
Thanks, I'll add them as a start. This clears up some of
my above listed "problematic" cases.
>> there is one case which needs to
>> use the retrieved parameter in two places (win_reg),
>
> I do not see nay problem in win_regc.c but maybe I'm missing sth.
> Now HB_TCHAR_*() macros are use only in one function:
> HB_FUNC( WIN_REGQUERYVALUEEX )
> {
> LPTSTR lpKey = HB_TCHAR_CONVTO( hb_parcx( 2 ) );
> [...]
> HB_TCHAR_FREE( lpKey );
> }
>
> and should be simply replaced with:
> HB_FUNC( WIN_REGQUERYVALUEEX )
> {
> void * hKey;
> LPCTSTR lpKey = HB_PARSTRDEF( 2, &hKey, NULL );
> [...]
> hb_strfree( hKey );
> }
>
> or better by:
> HB_FUNC( WIN_REGQUERYVALUEEX )
> {
> void * hKey;
> LPCTSTR lpKey = HB_PARSTR( 2, &hKey, NULL );
> [...]
> if( lpKey && RegQueryValueEx( ( HKEY ) hb_parptr( 1 ),
> [...]
> hb_strfree( hKey );
> }
That's fine, thanks.
>> and some more. Probably some additional macros are
>> needed?
>
> Probably yes but without exact real life examples it's hard for me
> to guess which ones.
I'll try to modify everything which I can, and pls
check remaining ones.
Brgds,
Viktor
_______________________________________________
Harbour mailing list (attachment size limit: 40KB)
[email protected]
http://lists.harbour-project.org/mailman/listinfo/harbour
