On Mon, Nov 26, 2012 at 8:54 AM, Nikolay Sivov <bungleh...@gmail.com> 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