Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Juergen Gross <jgross@xxxxxxxx>]

On 01.12.22 11:12, Julien Grall wrote:
> On 01/12/2022 10:08, Juergen Gross wrote:
> > On 01.12.22 10:31, Julien Grall wrote:
> > > (+ A few people)
> > >
> > > On 01/12/2022 09:21, Edwin Torok wrote:
> > > > > On 1 Dec 2022, at 09:11, Julien Grall <julien@xxxxxxx> wrote:
> > > > >
> > > > > Hi Edwin,
> > > > >
> > > > > The title should have "OCaml" to clarify that .clang-format is not added at 
> > > > > the root level.
> > > > Sure, I'll update that when I resend.
> > > >
> > > > > On 30/11/2022 17:32, Edwin Török wrote:
> > > > > > Add a .clang-format configuration that tries to match CODING_STYLE where
> > > > > > possible.
> > > > > > I was not able to express the special casing of braces after 'do'
> > > > > > though, this can only be controlled generally for all control
> > > > > > statements.
> > > > > > It is imperfect, but should be better than the existing bindings, which
> > > > > > do not follow Xen coding style.
> > > > > Right, from previous discussion, I was under the impression that it 
> > > > > requires some work to write a clang-format file for Xen.
> > > > > I am hopeful that some day we will have a proper one. In fact, we have been 
> > > > > discussing about this as part of MISRA (+ Stefano).
> > > > > > Add this to tools/ocaml first because:
> > > > > > * there are relatively few C files here, and it is a good place to start with
> > > > > > * it'd be useful to make these follow Xen's CODING_STYLE
> > > > > > (which they currently do not because they use tabs for example)
> > > > > > * they change relatively infrequently, so shouldn't cause issues with
> > > > > >    backporting security fixes (could either backport the reindentation
> > > > > >    patch too, or use git cherry-pick with `-Xignore-space-change`)
> > > > > > Once this is used it'll need inserting some '#include <stdint.h>', 
> > > > > > otherwise xs_wire.h
> > > > > > fails to compile due to the missing uint32_t define.
> > > > > At first I was a bit concerned with this paragraph because a coding style 
> > > > > should not impact compilation. But I guess that's because the format will 
> > > > > convert u32 to uint32_t. Is that correct?
> > > > > If so, I would expand the paragraph to explicit say that, per the coding 
> > > > > styel, u32 will be converted to uint32_t.
> > > >
> > > > clang-format rearranges the order of '#include' in C files, it shouldn't 
> > > > convert types.
> > > > But rearranging (sorting) includes can sometimes reveal bugs where the code 
> > > > only happened to compile because the includes were done in a certain order
> > > > (e.g. we included something that included stdint.h, therefore the next 
> > > > include line worked, but if you swap the include order that no longer 
> > > > works), i.e.:
> > > > #include "c.h"
> > > > #include "b.h"
> > > >
> > > > vs
> > > >
> > > > /* post formatting */
> > > > #include "b.h"
> > > > #include "c.h"
> > > >
> > > > Where c.h includes a.h, and b.h depends on declarations from a.h, then prior 
> > > > to reformat the code compiles, and afterwards it doesn't.
> > > Thanks for the detailed information, I think some of it needs to be 
> > > summarized in the commmit message.
> > > > Which can be fixed by adding this to the C file (and then regardless of 
> > > > include order of the other 2 it compiles):
> > > > #include <a.h>
> > > This would not work if the file were called "d.h" rather than "a.h" because 
> > > the clang format would re-order it. So...
> > > > Or by fixing b.h to include a.h itself it it depends on it.
> > > ... this is the proper way to fix it.
> > >
> > > > Perhaps this'd better be fixed in xs_wire.h itself to include all its 
> > > > dependencies, but that is a publicly installed header, and there might be a 
> > > > reason why it doesn't include stdint.h.
> > > I am not aware of any restrictions and at least one public headers already 
> > > include <stdint.h>. I am CCed a few more people to get an opinion.
> > I don't think xs_wire.h should include stdint.h. This will result in a conflict
> > e.g. in the Linux kernel.
> AFAIU, Linux has its own copy of the headers. Is that correct?
Yes.

> > We might want to add a comment to xs_wire.h like the one in ring.h in order to
> > document the requirement of the type definition of uint32_t.
> The problem with this approach is you made more difficult for any userspace 
> application to use the headers. So I would argue that the Linux copy can remove 
> "stdint.h" if needed.
Today there is exactly one public header including stdint.h, and I'd argue
that this was a mistake.

xs_wire.h is especially rather uninteresting for any user space application
but a Xenstore implementation. All consumers of xs_wire.h are probably
either in the Xen tree, or operating system kernels. User space applications
should use libxenstore for accessing the Xenstore, so I don't see an
advantage in breaking the usual philosophy of the Xen public headers NOT
including external headers like stdint.h.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature