Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On Fri, Apr 1, 2011 at 6:25 PM, Michael Roth wrote:
> On 04/01/2011 12:01 PM, Stefan Hajnoczi wrote:
>>
>> On Fri, Apr 01, 2011 at 11:55:39AM -0500, Michael Roth wrote:
>>>
>>> I'd prefer to only document "strict" guidelines, and treat
>>> checkpatch.pl warnings ("suggestions") as an extra "reward" you get
>>> for taking the time to run it.
>>
>> I don't want to be punished for running checkpatch.pl like I'm supposed
>> to while those who don't can get away with more.
>
> You're not! These are extra morsels of goodness :)
>
>>
>> A --pedantic mode would be fine although probably no one besides the
>> author would use it :).
>
> True :) But you're right, this is probably the better approach. How bout:
>
> --warnings: print coding style warnings in addition to errors, and exit
> failure if encountered
>
> Then default to suppressing warning statements, and --no-fail-on-warn
> behavior.
Sounds good to me :).
Stefan
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On 04/01/2011 12:01 PM, Stefan Hajnoczi wrote:
On Fri, Apr 01, 2011 at 11:55:39AM -0500, Michael Roth wrote:
I'd prefer to only document "strict" guidelines, and treat
checkpatch.pl warnings ("suggestions") as an extra "reward" you get
for taking the time to run it.
I don't want to be punished for running checkpatch.pl like I'm supposed
to while those who don't can get away with more.
You're not! These are extra morsels of goodness :)
A --pedantic mode would be fine although probably no one besides the
author would use it :).
True :) But you're right, this is probably the better approach. How bout:
--warnings: print coding style warnings in addition to errors, and exit
failure if encountered
Then default to suppressing warning statements, and --no-fail-on-warn
behavior.
The stuff that gets reported by the default invocation needs to matter,
otherwise checkpatch.pl isn't useful and people will bypass it.
Stefan
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On 04/01/2011 11:58 AM, Peter Maydell wrote: On 1 April 2011 17:55, Michael Roth wrote: But there *are* some warnings that make sense to complain about without saying "you can't do this", like extern's in .c files: some cases are exceptional. I'd treat everything checkpatch says as a warning anyway, because it gets confused by things like macros often enough that you can't guarantee that everything it thinks is an error is truly an error. Well, we always have the option of ignoring the return code and just looking at the output. Would be nice to be able to automate the check somewhat reliably though, and just override the false positives when they pop up. -- PMM
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On Fri, Apr 01, 2011 at 11:55:39AM -0500, Michael Roth wrote:
> I'd prefer to only document "strict" guidelines, and treat
> checkpatch.pl warnings ("suggestions") as an extra "reward" you get
> for taking the time to run it.
I don't want to be punished for running checkpatch.pl like I'm supposed
to while those who don't can get away with more.
A --pedantic mode would be fine although probably no one besides the
author would use it :).
The stuff that gets reported by the default invocation needs to matter,
otherwise checkpatch.pl isn't useful and people will bypass it.
Stefan
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On 1 April 2011 17:55, Michael Roth wrote: > But there *are* some warnings that make sense to complain about without > saying "you can't do this", like extern's in .c files: some cases are > exceptional. I'd treat everything checkpatch says as a warning anyway, because it gets confused by things like macros often enough that you can't guarantee that everything it thinks is an error is truly an error. -- PMM
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On 04/01/2011 11:16 AM, Peter Maydell wrote:
On 1 April 2011 16:59, Stefan Hajnoczi wrote:
On Fri, Apr 1, 2011 at 4:52 PM, Peter Maydell wrote:
On 1 April 2011 16:20, Michael Roth wrote:
We also make C99 //comments a warning instead of an error, since they
don't actually violate QEMU's coding guidelines.
We should either update the guidelines or fix the script...
There are a whole bunch of // in the codebase. I prefer /* */ but as
it stands I think // should not even raise a warning.
I don't care much either, really. I just don't think we should be
introducing random coding standards rules by the back door because
checkpatch happens to complain about them.
Whether or not // comments should also be a warning...I'm not sure. It
seems like a reasonable "suggestion" to make though, since mixed comment
styles makes code look nastier. I could also go either way though...
But there *are* some warnings that make sense to complain about without
saying "you can't do this", like extern's in .c files: some cases are
exceptional.
I'd prefer to only document "strict" guidelines, and treat checkpatch.pl
warnings ("suggestions") as an extra "reward" you get for taking the
time to run it.
-- PMM
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On 1 April 2011 16:59, Stefan Hajnoczi wrote: > On Fri, Apr 1, 2011 at 4:52 PM, Peter Maydell > wrote: >> On 1 April 2011 16:20, Michael Roth wrote: >>> We also make C99 //comments a warning instead of an error, since they >>> don't actually violate QEMU's coding guidelines. >> >> We should either update the guidelines or fix the script... > > There are a whole bunch of // in the codebase. I prefer /* */ but as > it stands I think // should not even raise a warning. I don't care much either, really. I just don't think we should be introducing random coding standards rules by the back door because checkpatch happens to complain about them. -- PMM
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On Fri, Apr 1, 2011 at 4:52 PM, Peter Maydell wrote: > On 1 April 2011 16:20, Michael Roth wrote: >> We also make C99 //comments a warning instead of an error, since they >> don't actually violate QEMU's coding guidelines. > > We should either update the guidelines or fix the script... There are a whole bunch of // in the codebase. I prefer /* */ but as it stands I think // should not even raise a warning. Stefan
Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail
On 1 April 2011 16:20, Michael Roth wrote: > We also make C99 //comments a warning instead of an error, since they > don't actually violate QEMU's coding guidelines. We should either update the guidelines or fix the script... -- PMM
