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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

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

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>

Or by fixing b.h to include a.h itself it it depends on 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.
(neither u32, nor uint32_t is standard C, both require a header to be included 
for them to be available)

Best regards,
--Edwin
> 
> Cheers,
> 
> -- 
> Julien Grall