Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Loïc Maury]

Hello,

After the various comments, I have modified the patch.

What do you think ?

Thank You

Loïc

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Dmitry Timoshkov]

Loïc Maury  wrote:
> After the various comments, I have modified the patch.

First of all set your tab size to 8, and ask your editor to not use tabs
at all.
> - TRACE("(%s, %s, %p, %d, %d)\n",debugstr_w(printer->name)
> -
>   ,debugstr_w(printer->printername)
> -
>   ,printer->backend_printer
> -
>   ,printer->queue->ref
> -
>   ,list_count(&printer->queue->jobs));

TRACE() with the API parameters usually is the very first statement in
the API implementation, comma should be placed at the end of the statement,
not before.
> -
> 		GetPrinterW(hPrinter, 2, NULL, 0, &needed);
> -
> 		pi2 = HeapAlloc(GetProcessHeap(), 0, needed);
> -
> 		GetPrinterW(hPrinter, 2, (LPBYTE)pi2, needed, &needed);

You need to check the return value of GetPrinterW() and handle the errors.
> - if(pi2)
> -
> HeapFree(GetProcessHeap(), 0, pi2);

NULL check before HeapFree() is not needed.
> - TRACE("return %d\n", ret);

This trace is redundant.

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Loïc Maury]

Hello Mr.Timoshkov,

Thank you for your reply.
> Loïc Maury  wrote:
> > After the various comments, I have modified the patch.
>
> First of all set your tab size to 8, and ask your editor to not use tabs
> at all.

I have modified my editor, but I don't know if it 's correct now ?
> > - TRACE("(%s, %s, %p, %d, %d)\n",debugstr_w(printer->name)
> > -
> >   ,debugstr_w(printer->printername)
> > -
> >   ,printer->backend_printer
> > -
> >   ,printer->queue->ref
> > -
> >   ,list_count(&printer->queue->jobs));
>
> TRACE() with the API parameters usually is the very first statement in
> the API implementation, comma should be placed at the end of the statement,
> not before.

Ok, I have remplaced this TRACE().
> > -
> > 		GetPrinterW(hPrinter, 2, NULL, 0,&needed);
> > -
> > 		pi2 = HeapAlloc(GetProcessHeap(), 0, needed);
> > -
> > 		GetPrinterW(hPrinter, 2, (LPBYTE)pi2, needed,&needed);
>
> You need to check the return value of GetPrinterW() and handle the errors.

Ok
> > - if(pi2)
> > -
> > HeapFree(GetProcessHeap(), 0, pi2);
>
> NULL check before HeapFree() is not needed.

Ok, I have removed the NULL check.
> > - TRACE("return %d\n", ret);
>
> This trace is redundant.

Ok, I have removed this TRACE().

I have make an other patch.

Thank you

Loïc

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Andrew Eikum]

On 02/08/2011 10:48 AM, Loïc Maury wrote:
> > First of all set your tab size to 8, and ask your editor to not use tabs
> >  at all.
>
> I have modified my editor, but I don't know if it 's correct now ?

The indentation looks fine to me now. You've got an extra newline after 
the "if(printer->doc)" block and before the "else." The formatting on 
the TRACE statement is still bizarre. Fix the commas and put it on just 
a couple of lines. No reason for one line and a ton of whitespace for 
each parameter. See line 1944 for an example.

Otherwise, two little things I noticed.

First, you must also check the return value for HeapAlloc().

Second, you should be using the LIST_FOR_EACH_ENTRY_SAFE macro, which 
will eliminate the local cursor and cursor2 variables and make the code 
a little more simple. (Unless there's something I'm missing?)

Almost done, nice work :)

Andrew

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Loïc Maury]

Hello Mr.Eikum,

Thank you for your reply.

On 08/02/11 18:13, Andrew Eikum wrote:
> On 02/08/2011 10:48 AM, Loïc Maury wrote:
> > > First of all set your tab size to 8, and ask your editor to not use 
> > > tabs
> > >  at all.
> >
> > I have modified my editor, but I don't know if it 's correct now ?
>
> The indentation looks fine to me now. You've got an extra newline 
> after the "if(printer->doc)" block and before the "else." The 
> formatting on the TRACE statement is still bizarre. Fix the commas and 
> put it on just a couple of lines. No reason for one line and a ton of 
> whitespace for each parameter. See line 1944 for an example.

Ok, it's correct now ?
> Otherwise, two little things I noticed.
>
> First, you must also check the return value for HeapAlloc().

Ok
> Second, you should be using the LIST_FOR_EACH_ENTRY_SAFE macro, which 
> will eliminate the local cursor and cursor2 variables and make the 
> code a little more simple. (Unless there's something I'm missing?)

Ok, I have modified and removed cursor and cursor2 by job and next_job, 
and list_remove(cursor) by list_remove(&job->entry).
> Almost done, nice work :)

Thank you

Loïc
> Andrew

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Andrew Eikum]

On 02/09/2011 09:09 AM, Loïc Maury wrote:
> > The indentation looks fine to me now. You've got an extra newline
> >  after the "if(printer->doc)" block and before the "else." The
> >  formatting on the TRACE statement is still bizarre. Fix the commas and
> >  put it on just a couple of lines. No reason for one line and a ton of
> >  whitespace for each parameter. See line 1944 for an example.
>
> Ok, it's correct now ?

The content looks fine, but there are still formatting problems in your 
patch. You're still using commas strangely in your wrapped TRACE 
statement. Your editor is broken again, and you are now mixing tabs and 
spaces. A few lines still have whitespace at the end.

These are things you should verify yourself before sending it off for 
someone else to look at.

Andrew

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Loïc Maury]

Hello Mr.Eikum,

On 09/02/11 16:30, Andrew Eikum wrote:
> On 02/09/2011 09:09 AM, Loïc Maury wrote:
> > > The indentation looks fine to me now. You've got an extra newline
> > >  after the "if(printer->doc)" block and before the "else." The
> > >  formatting on the TRACE statement is still bizarre. Fix the commas and
> > >  put it on just a couple of lines. No reason for one line and a ton of
> > >  whitespace for each parameter. See line 1944 for an example.
> >
> > Ok, it's correct now ?
>
> The content looks fine, but there are still formatting problems in 
> your patch. You're still using commas strangely in your wrapped TRACE 
> statement. Your editor is broken again, and you are now mixing tabs 
> and spaces. A few lines still have whitespace at the end.
>
> These are things you should verify yourself before sending it off for 
> someone else to look at.

Sorry for this, now I have verified with git diff and git diff --check, 
and I have no trailing whitespace.

I think it's correct.

Thank you

Loïc
> Andrew

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Nicolas Le Cam]

2011/2/8 Loïc Maury
> Hello Mr.Timoshkov,
>
> Thank you for your reply.
> > Loīc Maury  wrote:
> >
> > After the various comments, I have modified the patch.
> > >
> >
> > First of all set your tab size to 8, and ask your editor to not use tabs
> > at all.
>
> I have modified my editor, but I don't know if it 's correct now ?
> > -
> >   TRACE("(%s, %s, %p, %d, %d)\n",debugstr_w(printer->name)
> > > -
> > >             ,debugstr_w(printer->printername)
> > > -
> > >             ,printer->backend_printer
> > > -
> > >             ,printer->queue->ref
> > > -
> > >             ,list_count(&printer->queue->jobs));
> >
> > TRACE() with the API parameters usually is the very first statement in
> > the API implementation, comma should be placed at the end of the
> > statement,
> > not before.
>
> Ok, I have remplaced this TRACE().
> > -
> >                           GetPrinterW(hPrinter, 2, NULL, 0,&needed);
> > > -
> > >                           pi2 = HeapAlloc(GetProcessHeap(), 0,
> > >
> > > needed);
> > > -
> > >                           GetPrinterW(hPrinter, 2, (LPBYTE)pi2,
> > >
> > > needed,&needed);
> >
> > You need to check the return value of GetPrinterW() and handle the errors.
>
> Ok
> > -
> >   if(pi2)
> > > -
> > >           HeapFree(GetProcessHeap(), 0, pi2);
> >
> > NULL check before HeapFree() is not needed.
>
> Ok, I have removed the NULL check.
> - TRACE("return %d\n", ret);
> > >
> >
> > This trace is redundant.
>
> Ok, I have removed this TRACE().
>
> I have make an other patch.
>
> Thank you
>
> Loīc

Hi Loïc,

In addition to Dmitry and Andrew comments, you're adding a lot of trailing
spaces (git apply output : warning: squelched 22 whitespace errors warning:
27 lines add whitespace errors.), there's also no need for an extra newline
after the end label.

Try to Implement my first Stub function - AbortPrinter() - (try 2).

[Loïc Maury]

Hello Mr.Le Cam

On Tue, Feb 8, 2011 at 10:07 PM, Nicolas Le Cam wrote:
> 2011/2/8 Loïc Maury
> > Hello Mr.Timoshkov,
> >
> > Thank you for your reply.
> > > Loīc Maury  wrote:
> > >
> > > After the various comments, I have modified the patch.
> > > >
> > >
> > > First of all set your tab size to 8, and ask your editor to not use tabs
> > > at all.
> >
> > I have modified my editor, but I don't know if it 's correct now ?
> > > -
> > >   TRACE("(%s, %s, %p, %d, %d)\n",debugstr_w(printer->name)
> > > > -
> > > >             ,debugstr_w(printer->printername)
> > > > -
> > > >             ,printer->backend_printer
> > > > -
> > > >             ,printer->queue->ref
> > > > -
> > > >             ,list_count(&printer->queue->jobs));
> > >
> > > TRACE() with the API parameters usually is the very first statement in
> > > the API implementation, comma should be placed at the end of the
> > > statement,
> > > not before.
> >
> > Ok, I have remplaced this TRACE().
> > > -
> > >                           GetPrinterW(hPrinter, 2, NULL,
> > > > 0,&needed);
> > > > -
> > > >                           pi2 = HeapAlloc(GetProcessHeap(), 0,
> > > >
> > > > needed);
> > > > -
> > > >                           GetPrinterW(hPrinter, 2, (LPBYTE)pi2,
> > > >
> > > > needed,&needed);
> > >
> > > You need to check the return value of GetPrinterW() and handle the
> > > errors.
> >
> > Ok
> > > -
> > >   if(pi2)
> > > > -
> > > >           HeapFree(GetProcessHeap(), 0, pi2);
> > >
> > > NULL check before HeapFree() is not needed.
> >
> > Ok, I have removed the NULL check.
> > - TRACE("return %d\n", ret);
> > > >
> > >
> > > This trace is redundant.
> >
> > Ok, I have removed this TRACE().
> >
> > I have make an other patch.
> >
> > Thank you
> >
> > Loīc
>
> Hi Loïc,
>
> In addition to Dmitry and Andrew comments, you're adding a lot of trailing
> spaces (git apply output : warning: squelched 22 whitespace errors warning:
> 27 lines add whitespace errors.), there's also no need for an extra newline
> after the end label.

Now, The patch is correct ?

Thank you

Loïc
> --
> Nicolas Le Cam