Re: [1/2] ole32: Add helper for string table memory freeing

[Nikolay Sivov]

On 11/26/2012 01:01, Frédéric Delanoy wrote:
> dlls/ole32/filemoniker.c | 44 ++++++++++++++++++--------------------------
>   1 file changed, 18 insertions(+), 26 deletions(-)
>
> diff --git a/dlls/ole32/filemoniker.c b/dlls/ole32/filemoniker.c
> index b3a5cc2..688f6b8 100644
> --- a/dlls/ole32/filemoniker.c
> +++ b/dlls/ole32/filemoniker.c
> @@ -657,6 +657,16 @@ FileMonikerImpl_Reduce(IMoniker* iface, IBindCtx* pbc, DWORD dwReduceHowFar,
>       return MK_S_REDUCED_TO_SELF;
>   }
>
> +static void
> +FileMonikerImpl_FreeStringTable(LPOLESTR* stringTable)
> +{
> - int i;
> -
> - for (i=0; stringTable[i]!=NULL; i++)
> -
>    CoTaskMemFree(stringTable[i]);
> - CoTaskMemFree(stringTable);
>
> +}
> -

Could you please adjust a naming for that to something like 
free_stringtable() cause it doesn't really access file moniker in this 
helper. Also stringTable[i]!=NULL could be shortened to stringTable[i].
>       CoTaskMemFree(str1);
>       CoTaskMemFree(str2);
>
> @@ -1004,17 +1010,9 @@ FileMonikerImpl_CommonPrefixWith(IMoniker* iface,IMoniker* pmkOther,IMoniker** p
>           {
>               for(i=0;i<sameIdx;i++)
>                   strcatW(commonPath,stringTable1[i]);
> -
> -
>        for(i=0;i<nb1;i++)
> -
>            CoTaskMemFree(stringTable1[i]);
> -
> -
>        CoTaskMemFree(stringTable1);
> -
> -
>        for(i=0;i<nb2;i++)
> -
>            CoTaskMemFree(stringTable2[i]);
> -
> -
>        CoTaskMemFree(stringTable2);
> -
> -
> -
>        FileMonikerImpl_FreeStringTable(stringTable1);
> -
>        FileMonikerImpl_FreeStringTable(stringTable2);

You sure it's an equivalent change? I mean regarding index boundaries above.

Also while you're at it FileMonikerImpl_DecomposePath() needs to be 
properly fixed - it returns HRESULT, not it. And surrounding code relies 
on FAILED() mask to get this index boundary from return value, this is 
really broken.

[1/2] ole32: Add helper for string table memory freeing

[Frédéric Delanoy]

On Mon, Nov 26, 2012 at 8:54 AM, Nikolay Sivov  wrote:
> On 11/26/2012 01:01, Frédéric Delanoy wrote:
> > dlls/ole32/filemoniker.c | 44
> > ++++++++++++++++++--------------------------
> >   1 file changed, 18 insertions(+), 26 deletions(-)
> >
> > diff --git a/dlls/ole32/filemoniker.c b/dlls/ole32/filemoniker.c
> > index b3a5cc2..688f6b8 100644
> > --- a/dlls/ole32/filemoniker.c
> > +++ b/dlls/ole32/filemoniker.c
> > @@ -657,6 +657,16 @@ FileMonikerImpl_Reduce(IMoniker* iface, IBindCtx*
> > pbc, DWORD dwReduceHowFar,
> >       return MK_S_REDUCED_TO_SELF;
> >   }
> >   +static void
> > +FileMonikerImpl_FreeStringTable(LPOLESTR* stringTable)
> > +{
> > - int i;
> > -
> > - for (i=0; stringTable[i]!=NULL; i++)
> > -
> >    CoTaskMemFree(stringTable[i]);
> > - CoTaskMemFree(stringTable);
> >
> > +}
> > -
>
> Could you please adjust a naming for that to something like
> free_stringtable() cause it doesn't really access file moniker in this
> helper.

OK
> Also stringTable[i]!=NULL could be shortened to stringTable[i].

I know but it's mostly a matter of style. I prefer the '!= NULL'
notation when accessing it "array-style", and any sane compiler would
optimize it anyway.
> >         CoTaskMemFree(str1);
> >       CoTaskMemFree(str2);
> >
> > @@ -1004,17 +1010,9 @@ FileMonikerImpl_CommonPrefixWith(IMoniker*
> > iface,IMoniker* pmkOther,IMoniker** p
> >           {
> >               for(i=0;i<sameIdx;i++)
> >                   strcatW(commonPath,stringTable1[i]);
> > -
> > -
> >        for(i=0;i<nb1;i++)
> > -
> >            CoTaskMemFree(stringTable1[i]);
> > -
> > -
> >        CoTaskMemFree(stringTable1);
> > -
> > -
> >        for(i=0;i<nb2;i++)
> > -
> >            CoTaskMemFree(stringTable2[i]);
> > -
> > -
> >        CoTaskMemFree(stringTable2);
> > -
> > -
> > -
> >        FileMonikerImpl_FreeStringTable(stringTable1);
> > -
> >        FileMonikerImpl_FreeStringTable(stringTable2);
>
> You sure it's an equivalent change? I mean regarding index boundaries above.

It's equivalent.
> Also while you're at it FileMonikerImpl_DecomposePath() needs to be
> properly fixed - it returns HRESULT, not it. And surrounding code relies on
> FAILED() mask to get this index boundary from return value, this is really
> broken.

Yes, but that probably warrants a patch of its own, to keep this one
as self-contained/atomic as possible.
I'll look into that

Frédéric