Hi Alistair,
Alistair Leslie-Hughes wrote:
Hi,
Could I get some feedback on this patch please?
+ HRESULT hr = E_INVALIDARG;
+
+ TRACE("(%p)->(%p, %p)\n", This, pAdvSink, pdwConnection);
+
+ if(!pdwConnection || !pAdvSink)
+ return hr;
+
+ if(!This->holder)
+ {
+ hr = CreateOleAdviseHolder(&This->holder);
+ if(FAILED(hr))
+ ERR("CreateOleAdviseHolder failed\n");
+ }
+
+ if(hr == S_OK && This->holder)
+ {
+ hr = IOleAdviseHolder_Advise(This->holder, pAdvSink, pdwConnection);
+ }
+
+ return hr;
It's a matter of taste, but you complicate things here. You could
achieve the same effect easier:
TRACE("(%p)->(%p, %p)\n", This, pAdvSink, pdwConnection);
if(!pdwConnection || !pAdvSink)
return E_INVALIDARG;
if(!This->holder) {
HRESULT hr = CreateOleAdviseHolder(&This->holder);
if(FAILED(hr)) {
ERR("CreateOleAdviseHolder failed\n");
return hr;
}
}
return IOleAdviseHolder_Advise(This->holder, pAdvSink, pdwConnection);
Also you don't really add support for IAdviseSink. You only store and
never use it and your test don't document missing functionality. It
would be nice not to call Unadvise just after Advise, but keep it
advised and properly test (with todo_wine for now). It's easy to do by
adding something like
trace(#func "\n"); \
to CHECK_EXPECT2 and running the test on Windows. Then you can easily
guess when the callback should be called and add corresponding
SET_EXPECT/CHECK_CALLED macros. Note that, although it's not needed for
tests to success, these macros are kept in order of calling as much as
possible for documentation purposes.
If you're not going to extend tests, please leave FIXME messages.
Otherwise you hide the problem with no evidence.
Thanks,
Jacek
