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.