Re: [PATCH] shlwapi: initial implement of SHAutoComplete

[Nikolay Sivov]

On 5/23/2010 05:09, Hirofumi Katayama wrote:
> See attachment.

Hi, some comments here.
> +/****************************************************************************/
> -
>
> +static const WCHAR szMRUList[] = L"MRUList";
> +static const WCHAR szRunMRU[] =
> - L"Software\Microsoft\Windows\CurrentVersion\Explorer\RunMRU";
> -

Don't use this L"" specifier for strings, use array initialization style 
instead.
> +void *my_malloc(UINT size)
> +{
> - return HeapAlloc(GetProcessHeap(), 0, size);
>
> +}
> -
>
> +void *my_realloc(void *p, UINT size)
> +{
> - if (p == NULL)
> -
>    return my_malloc(size);
> - return HeapReAlloc(GetProcessHeap(), 0, p, size);
>
> +}
> -
>
> +void my_free(void *p)
> +{
> - HeapFree(GetProcessHeap(), 0, p);
>
> +}
> -

This isn't very useful, I think it's not a pain to use direct calls for 
this file.
> +#define malloc my_malloc
> +#define realloc my_realloc
> +#define free my_free
> +#define _wcsdup my__wcsdup

Don't do that.
> -
>
> +STDMETHODIMP IEnumString_fnQueryInterface(IEnumString *pEnumString, REFIID riid, void** ppvObject);
> +STDMETHODIMP_(ULONG) IEnumString_fnAddRef(IEnumString *pEnumString);
> +STDMETHODIMP_(ULONG) IEnumString_fnRelease(IEnumString *pEnumString);
> +STDMETHODIMP IEnumString_fnNext(IEnumString *pEnumString, ULONG celt, LPOLESTR *rgelt, ULONG *pceltFetched);
> +STDMETHODIMP IEnumString_fnSkip(IEnumString *pEnumString, ULONG celt);
> +STDMETHODIMP IEnumString_fnReset(IEnumString *pEnumString);
> +STDMETHODIMP IEnumString_fnClone(IEnumString *pEnumString, IEnumString **ppenum);

STDMETHODIMP isn't used in Wine code, use explicit WINAPI please. Also 
forward declaration for methods isn't used, usually a vtable is
initialized just after all methods bodies.
> +typedef struct
> +{
> - IEnumStringVtbl *lpVtbl;
> - LONG        m_lRef;
> - IMalloc *   m_pMalloc;
> - INT         m_i;
> - INT         m_c;
> - LPWSTR *    m_strings;
>
> +} EnumStringImpl;

Please avoid this MFC style like m_* for members, and use more 
descriptive names.
> +EnumStringImpl *IEnumString_construct(VOID)
> +{
> - EnumStringImpl *pEnumString;
> -
> - pEnumString = (EnumStringImpl *) malloc(sizeof(EnumStringImpl));
> - if (pEnumString != NULL)
> - {
> -
>    pEnumString->lpVtbl =&EnumStringVtbl;
> -
>    pEnumString->m_lRef = 0;
> -
>    pEnumString->m_pMalloc = NULL;
> -
>    SHGetMalloc(&pEnumString->m_pMalloc);
> -
>    pEnumString->m_i = 0;
> -
>    pEnumString->m_strings = GetMRUStrings(&pEnumString->m_c);
> - }
> - return pEnumString;
>
> +}

This is wrong. Initial refcount should be 1.
> +EnumStringImpl *IEnumString_construct(VOID)
> -
>
> +VOID IEnumString_destruct(EnumStringImpl *pEnumString)

Should be static both, and methods too of course.
> -
>    pEnumString->lpVtbl->AddRef(pEnumString);

No need for that, use defined macros to call methods from vtables.
> +STDMETHODIMP_(ULONG) IEnumString_fnAddRef(IEnumString *pEnumString)
> +{
> - EnumStringImpl *this = (EnumStringImpl *) pEnumString;
> - this->m_lRef++;
> - return (ULONG) this->m_lRef;
>
> +}

You should use interlocked increment here (same for _Release()).
> - pProp->fnOldWndProc = (WNDPROC) SetWindowLongPtr(hwndEdit, GWLP_WNDPROC, (LONG_PTR) AutoCompleteEditWndProc);

Did you ever build it? I think we have a blocking things for using 
implicit A/W calls.