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