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

[Julien Grall <julien@xxxxxxx>]

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?

> 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.
Cheers,

--
Julien Grall