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

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall