Patch feedback requested for OleCreatePropertyFrame()
Wolfram Sang
wolfram at the-dreams.de
Fri Jan 8 03:27:24 CST 2010
Geoffrey Hausheer wrote:
> Would someone mind reviewing this for style/content? I still have no
Can't say much about the content, but stylewise the pointer handling
could be improved. For example,
+ if ( (iface==0) || (ppvObject==0) )
should at least use NULL instead of 0 as both are pointers, but !ptr is
even better here. Accordingly
+ *ppvObject = 0;
should assign NULL.
Also pointer declaration is not consistent, note the asterisk:
+ OLEPropertyPageSite *this = (OLEPropertyPageSite *)iface;
[...]
+ IPropertyPageSite* iface,
(and there are two spaces here)
I don't know which way is preferred, but for readability it should be
the same throughout the file.
+ if (IsEqualGUID(&IID_IUnknown, riid))
+ *ppvObject = iface;
+ else if (IsEqualGUID(&IID_IPropertyPageSite, riid))
+ *ppvObject = iface;
if (a || b) ?
+static ULONG WINAPI PPS_Release(IPropertyPageSite* iface)
+{
+ OLEPropertyPageSite *this = (OLEPropertyPageSite *)iface;
+ ULONG ret;
+ TRACE("(%p)->(ref=%d)\n", this, this->ref);
+
+ /* Decrease the reference count for current interface */
+ ret = InterlockedDecrement(&this->ref);
+
+ return ret;
+}
ret is not needed here.
+ /* This was expected to work but didn't:
+ LONG baseunits = GetDialogBaseUnits();
+ basex = LOWORD(baseunits);
+ basey = HIWORD(baseunits);
+ */
Don't know the common wine-sense here, but without syntax-highlighting,
I think it is easy to miss that this code is disabled. I'd suggest
/*
*
*/
commenting or something similar which is immediately visible.
Those are my 2 cents, thanks for the work!
Regards,
Wolfram
More information about the wine-devel
mailing list