| Message ID | 1524592709-6553-2-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(expanding the CC to include everyone that get_maintainer suggests)
Ian Jackson writes ("[PATCH 01/16] checkpatch: Add xendevicemodel_handle to the list of types"):
> This avoids checkpatch misparsing (as statements) long function
> definitions or declarations, which sometimes start with constructs
> like this:
>
> static inline int xendevicemodel_relocate_memory(
> xendevicemodel_handle *dmod, domid_t domid, ...
>
> The type xendevicemodel_handle does not conform to Qemu CODING_STYLE,
> which would suggest CamelCase. However, it is a type defined by the
> Xen Project in xen.git. It would be possible to introduce a typedef
> to allow the qemu code to refer to it by a differently-spelled name,
> but that would obfuscate more than it would clarify.
This patch has been posted in substantially similar form quite a few
times now. Paolo Bonzini understandably suggested that renaming the
variable would be better but that's not within qemu's bailiwick as I
say above.
I think everything else in this series has a review and/or an ack.
So I would like to send a pull request.
Does someone want to review this patch ? Should I drop it and just
let checkpatch complain ? Shold I include it in my pull request
anyway ?
Thanks,
Ian.
On 04/26/2018 06:06 AM, Ian Jackson wrote: > (expanding the CC to include everyone that get_maintainer suggests) > > Ian Jackson writes ("[PATCH 01/16] checkpatch: Add xendevicemodel_handle to the list of types"): >> This avoids checkpatch misparsing (as statements) long function >> definitions or declarations, which sometimes start with constructs >> like this: >> >> static inline int xendevicemodel_relocate_memory( >> xendevicemodel_handle *dmod, domid_t domid, ... >> >> The type xendevicemodel_handle does not conform to Qemu CODING_STYLE, >> which would suggest CamelCase. However, it is a type defined by the >> Xen Project in xen.git. It would be possible to introduce a typedef >> to allow the qemu code to refer to it by a differently-spelled name, >> but that would obfuscate more than it would clarify. > > This patch has been posted in substantially similar form quite a few > times now. Paolo Bonzini understandably suggested that renaming the > variable would be better but that's not within qemu's bailiwick as I > say above. > > I think everything else in this series has a review and/or an ack. > So I would like to send a pull request. > > Does someone want to review this patch ? Should I drop it and just > let checkpatch complain ? Shold I include it in my pull request > anyway ? If no one has commented on what seems pretty trivial (especially since checkpatch.pl has no official maintainer, but is more of a "whoever-touched-it-last" file at the moment), then including the patch in your pull request is perfectly acceptable. As a maintainer, it is also perfectly acceptable for you to ignore false positives from checkpatch (although documenting it in the commit message and/or cover letter never hurts, when you are intentionally ignoring a false positive). But, as there has also been a recent patch to teach checkpatch about glib types [1], your patch makes sense (any merge conflict between your patch and that one will be obvious to resolve). So on that grounds, Reviewed-by: Eric Blake <eblake@redhat.com> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg04179.html
> If no one has commented on what seems pretty trivial (especially since > checkpatch.pl has no official maintainer, but is more of a > "whoever-touched-it-last" file at the moment), then including the patch > in your pull request is perfectly acceptable. As a maintainer, it is > also perfectly acceptable for you to ignore false positives from > checkpatch (although documenting it in the commit message and/or cover > letter never hurts, when you are intentionally ignoring a false positive). Thanks for the comprehensive response. > But, as there has also been a recent patch to teach checkpatch about > glib types [1], your patch makes sense (any merge conflict between your > patch and that one will be obvious to resolve). So on that grounds, > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks. I will rebase onto current master before sending a pull request, so I'll get to resolve that conflict. Regards, Ian.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index d52207a..5b8735d 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -266,6 +266,7 @@ our @typeList = ( qr{target_(?:u)?long}, qr{hwaddr}, qr{xml${Ident}}, + qr{xendevicemodel_handle}, ); # This can be modified by sub possible. Since it can be empty, be careful
This avoids checkpatch misparsing (as statements) long function definitions or declarations, which sometimes start with constructs like this: static inline int xendevicemodel_relocate_memory( xendevicemodel_handle *dmod, domid_t domid, ... The type xendevicemodel_handle does not conform to Qemu CODING_STYLE, which would suggest CamelCase. However, it is a type defined by the Xen Project in xen.git. It would be possible to introduce a typedef to allow the qemu code to refer to it by a differently-spelled name, but that would obfuscate more than it would clarify. CC: Eric Blake <eblake@redhat.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Daniel P. Berrange <berrange@redhat.com> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> --- v7: Added comment to commit message about why we are not renaming this type to CamelCase. v6.1: New patch --- scripts/checkpatch.pl | 1 + 1 file changed, 1 insertion(+)