Re: Implementation of EnumMoniker and Partial impl on MkParseDisplayName

[Mike McCormack]

Hi Jeff,

Jeff Latimer wrote:
> This is the first installment of the implementation of 
> MkParseDisplayName.  I would like feed back on style and usage.
>
> I think the implementation of the EnumMoniker interface looks ok.  
> Comments appreciated.

I like that you've taken the time to write a test case!  I don't know 
alot about monikers, but seeing that you've tested it gives me confidence...

The following comments are a crash course in Wine programming...
> +/***********************************************************************
> + *        EnmumMoniker_Next
> + */
> +HRESULT   WINAPI EnumMonikerImpl_Next(IEnumMoniker* iface, ULONG celt, IMoniker** rgelt, ULONG * pceltFetched)
> +{
> +//    DWORD i;
> +//    ULONG ref;

We avoid C++ comments in Wine code to keep compatability with older 
compilers.
> +    EnumMonikerImpl *This = (EnumMonikerImpl *)iface;
> +    TRACE("(%p) currentPos %d Tablastindx %d\n",This, (int)This->currentPos, (int)This->TabLastIndx);
> +    ULONG i;

You declared "i" above, but commented it out.   Try avoid putting dead 
code in comments.   If code isn't compiled, it will become stale and 
confuse people.
> +    rc = CoGetMalloc(1, (LPMALLOC *) &ppMalloc);
>      if (!(IsValidInterface((LPUNKNOWN) pbc)))
>   return E_INVALIDARG;
> +    usernamelen = (int) lstrlenW(szUserName); 	// overall string len
> +
>
> +    if (szUserName[char_cnt] == (WCHAR) '@') { 	// This is a progid not a file name

That's alot of casts.  I don't think you really need that many, do you?
> +    /* retrieve the requested number of moniker from the current position */
> +    for(i=0;((This->currentPos < This->TabLastIndx) && (i < celt));i++)
> +	rgelt[i]=(IMoniker*)This->TabMoniker[This->currentPos++].pmkObj;

Better to avoid tabs... and too many brackets. A space or two extra in 
the for( i=0; (This->... might make things look better.
> +	    if (szUserName[char_cnt] == '/' ||
> +	    	szUserName[char_cnt] == '?' ||
> +	    	szUserName[char_cnt] == '<' ||
> +	    	szUserName[char_cnt] == '>' ||
> +	    	szUserName[char_cnt] == '*' ||
> +	    	szUserName[char_cnt] == '|' ||
> +	    	szUserName[char_cnt] == ':')

Maybe that can be shortened to :
             if (strchr("/?<>*|:",szUserName[char_cnt]))
> +    todo_wine { ok(hr==0x800401e4, "MkParseDisplayName - Progid not implemented hr=%08x\n", (int) hr); }

0x800401e4 is defined as MK_E_SYNTAX.  It would be clearer to use the 
define rather than a number.

%08lx is probably what you want instead of casting hr to (int) to get 
rid of the printf parameter type warning.
> +
> +    IBindCtx_Release(pbc);
> +
> +    /* A bad drive */
> +    static const WCHAR wszDisplayName2[] = {'1',':','s','i','d',':',
>          '2','0','D','0','4','F','E','0','-','3','A','E','A','-','1','0','6','9','-','A','2','D','8','-','0','8','0','0','2','B','3','0','3','0','9','D',':',0};

Again, for compatability, we avoid inline declarations, even though gcc 
can deal with them fine.

Mike