Try to implement my first stub function - AbortPrinter() - (try 3)

[Loïc Maury]

Hello,

I try always to implement my first stub function - AbortPrinter.

The last time, I send into Wine patch mailling list, but the patch was 
not correct.

Now I think the format for the patch is correct (no whitespace, etc…).

More over, I have modified the value if(!job->portname) by if(!portname),

because after the test "if(!job->portname)", I print trace of portname, 
but before he was initialized

into the "if condition" and can to not enter into this condition.

What do you think ?

Thank you

Loïc

[Andrew Eikum]

Hi Loïc,

On 04/08/2011 07:54 AM, Loïc Maury wrote:
> I try always to implement my first stub function - AbortPrinter.

It's looking good. Before you submit this, you should add some tests. 
Take a look in <dlls/winspool.drv/tests/>. Demonstrate that 
AbortPrinter() on Windows behaves the same way as your function does. 
Make sure to test the value of GetLastError() on failure. You can run 
your tests through the testbot https://testbot.winehq.org/ before 
submitting. Look at other tests for some guidance.

Below are a couple of tiny suggestions.

+        ERR("The handle for the printer is invalid.\n");
+        TRACE("Document inside for job id : %d\n", printer->doc->job_id);
+        ERR("No document.\n");
+            TRACE("(hf : %p, job id : %d)\n", printer->doc->hf, 
printer->doc->job_id);
+            TRACE("portname : %s\n", debugstr_w(portname));
+                TRACE("Remove job from the list.\n");
+                TRACE("Remove document for the printer : %s.\n", 
debugstr_w(printer->printername));

You're outputting a lot of debug information. The ERRs aren't really 
errors, but just invalid values from the application which are handled 
appropriately. No problem here, so no need to ERR.

The other TRACE statements might be useful for debugging this particular 
function, but can clutter the output and make future maintenance more 
difficult. I'd remove them.

+    /* For each jobs, see if we have a job document in the double 
linked list */
+            /* Get portname. */
+            /* Cups Port */
+                /* The document for the printer is not useful anymore. */

These comments are entirely redundant. The code is perfectly clear, and 
the comments will just rot if this code is updated later. Don't bother 
with comments like these unless you're doing something very unclear.

+                retFromGetPrinter = GetPrinterW(hPrinter, 2, 
(LPBYTE)pi2, needed, &needed);
+                if(!retFromGetPrinter)
+                    goto end;

You can just do "if(!GetPrinterW(...))" and remove the local variable.

Thanks for contributing,
Andrew

[Loïc Maury]

Hello Andrew,
> Hi Loïc,
>
> On 04/08/2011 07:54 AM, Loïc Maury wrote:
> > I try always to implement my first stub function - AbortPrinter.
>
> It's looking good. Before you submit this, you should add some tests. 
> Take a look in <dlls/winspool.drv/tests/>. Demonstrate that 
> AbortPrinter() on Windows behaves the same way as your function does. 
> Make sure to test the value of GetLastError() on failure. You can run 
> your tests through the testbot https://testbot.winehq.org/ before 
> submitting. Look at other tests for some guidance.

Ok I will see that.
> Below are a couple of tiny suggestions.
> -
>    ERR("The handle for the printer is invalid.\n");
> -
>    TRACE("Document inside for job id : %d\n", 
>
> printer->doc->job_id);
> -
>    ERR("No document.\n");
> -
>        TRACE("(hf : %p, job id : %d)\n", printer->doc->hf, 
>
> printer->doc->job_id);
> -
>        TRACE("portname : %s\n", debugstr_w(portname));
> -
>            TRACE("Remove job from the list.\n");
> -
>            TRACE("Remove document for the printer : %s.\n", 
>
> debugstr_w(printer->printername));
>
> You're outputting a lot of debug information. The ERRs aren't really 
> errors, but just invalid values from the application which are handled 
> appropriately. No problem here, so no need to ERR.
>
> The other TRACE statements might be useful for debugging this 
> particular function, but can clutter the output and make future 
> maintenance more difficult. I'd remove them.

Ok I will remove this TRACE and ERR.
> - /* For each jobs, see if we have a job document in the double
>
> linked list */
> -
>        /* Get portname. */
> -
>        /* Cups Port */
> -
>            /* The document for the printer is not useful 
>
> anymore. */
>
> These comments are entirely redundant. The code is perfectly clear, 
> and the comments will just rot if this code is updated later. Don't 
> bother with comments like these unless you're doing something very 
> unclear.

Ok
> -
>            retFromGetPrinter = GetPrinterW(hPrinter, 2, 
>
> (LPBYTE)pi2, needed, &needed);
> -
>            if(!retFromGetPrinter)
> -
>                goto end;
>
> You can just do "if(!GetPrinterW(...))" and remove the local variable.

Yes the local variable is not necessary

Thank you for your help.

Loïc
> Thanks for contributing,
> Andrew

[James McKenzie]

On 4/8/11 5:54 AM, Loïc Maury wrote:
> Hello,
>
> I try always to implement my first stub function - AbortPrinter.

One nice comment here, very efficient use of the local label end.

James McKenzie