Re: usp10: [RESEND] Add ScriptItemize, ScriptShape and ScriptPlace functionality

[James Hawkins]

On 2/13/06, Jeff L  wrote:
> This patch is a resend that contains the correct file that combines the
> code and the tests, sent previously as two patches, into one patch.  It
> also has the .cvsignore for dlls/usp10/tests and fixes some issues with
> comments and dead code.

You'll have more luck getting this accepted if you send each function
implementation in separate patches.  Some comments about the patch:

if (psc) *psc = NULL;
     return 0;
+    HeapFree ( GetProcessHeap(), 0, *psc);
+    *psc = NULL;

You should keep the if (psc), because the user can send in a NULL psc.

-    FIXME("%p,%p\n",ppSp,piNumScripts);
+    FIXME("%p,%p,%p\n",ppSp, *ppSp, piNumScripts);

If this function is now implemented, it should be a TRACE and not a
FIXME.  If ppSp is NULL, this call to FIXME will cause a crash (see
next comment.)

-    if (piNumScripts) *piNumScripts = 0;
+/*  Set up a sensible default and intialise pointers  */
+    *piNumScripts = MAX_SCRIPTS;
+    *ppSp =  (SCRIPT_PROPERTIES *)Global_Script;

Same as above, need to keep the if (ptr) checks.

+    TRACE("ppSp:%p, *ppSp:%p, **ppSp:%p, %d\n", ppSp, *ppSp, **ppSp,
+                                                *piNumScripts);

If you *really* need to see all the values of these pointers, then you
have to check that each pointer above it is not NULL.

+    FIXME("(%p,%p,%p,%s,%d,%d, %p, %d, %p, %d): stub\n",  hdc, psc, pwcChars,

I dont know what this function is supposed to do, but it looks like
you've added quite a bit of functionality to this function.  If it's
not completely implemented, I would change it to semi-stub, so we know
it at least does something.  Same thing for the rest of the functions.

A good first step is to send the tests in the first patch, adding
todo_wine's where appropriate.  Next send each function implementation
in separate patches afterwards, removing todo_wine's from tests that
now pass.

--
James Hawkins