Try to implement my first stub function - AbortPrinter() - (try 3)
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
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
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
participants (3)
-
Andrew Eikum -
James McKenzie -
Loïc Maury