| Message ID | bf4013cdb5f3e66693551b5e45b3f975b5a48795.1669829264.git.edvin.torok@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OCaml bindings for hvm_param_get and xc_evtchn_status | expand |
Hi Edwin, The title should have "OCaml" to clarify that .clang-format is not added at the root level. 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. Cheers,
> On 1 Dec 2022, at 09:11, Julien Grall <julien@xen.org> 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
> On 30 Nov 2022, at 17:32, Edwin Török <edvin.torok@citrix.com> wrote: > > Add a .clang-format configuration that tries to match CODING_STYLE where > possible > [..] > Does not yet reformat any code. > > No functional change. > > Signed-off-by: Edwin Török <edvin.torok@citrix.com> > --- > tools/ocaml/.clang-format | 9 +++++++++ > 1 file changed, 9 insertions(+) > create mode 100644 tools/ocaml/.clang-format Acked-by: Christian Lindig <christian.lindig@citrix.com> I support this kind of automation. I also agree with the previous comment that the title should indicate that this is only going into a subtree. — C
(+ A few people) On 01/12/2022 09:21, Edwin Torok wrote: > > >> On 1 Dec 2022, at 09:11, Julien Grall <julien@xen.org> 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,
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@xen.org> 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. 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. Juergen
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@xen.org> 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,
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@xen.org> 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
On 01/12/2022 10:44, Juergen Gross wrote: > On 01.12.22 11:12, Julien Grall wrote: >>> 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. I think Edwin example is a pretty good justification for including stdint.h. If you have a coding style requiring to order header alphabetically, then the developer may not even be able to include stdint.h without any hackery (e.g. introducing a header that will always be before the Xen public headers). Cheers,
On 01.12.2022 11:47, Julien Grall wrote: > > > On 01/12/2022 10:44, Juergen Gross wrote: >> On 01.12.22 11:12, Julien Grall wrote: >>>> 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. I think so, too. >> 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. > > I think Edwin example is a pretty good justification for including > stdint.h. I disagree. The intention has always been for consumers to provide the basic C99 types by whatever suitable means they have. Note that in Xen we also have no stdint.h. > If you have a coding style requiring to order header alphabetically, > then the developer may not even be able to include stdint.h without any > hackery (e.g. introducing a header that will always be before the Xen > public headers). Just to indicate that commonly style requirements may be weaker than "fully alphabetic" - we don't request full ordering. What we request is that groups (xen/, asm/, public/) be ordered within any group, but we do not (afaia) demand ordering across groups (and indeed commonly we have asm/ after xen/). Jan
On 01/12/2022 11:30, Jan Beulich wrote: > On 01.12.2022 11:47, Julien Grall wrote: >> >> >> On 01/12/2022 10:44, Juergen Gross wrote: >>> On 01.12.22 11:12, Julien Grall wrote: >>>>> 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. > > I think so, too. > >>> 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. >> >> I think Edwin example is a pretty good justification for including >> stdint.h. > > I disagree. The intention has always been for consumers to provide the > basic C99 types by whatever suitable means they have. Note that in Xen > we also have no stdint.h. I really dislike when I have to find the dependency of an header. This is really a waste of time... If other disagree with that, then the strict minimum would be for this dependency to be recorded if it hasn't been done (I couldn't find anywhere). > >> If you have a coding style requiring to order header alphabetically, >> then the developer may not even be able to include stdint.h without any >> hackery (e.g. introducing a header that will always be before the Xen >> public headers). > > Just to indicate that commonly style requirements may be weaker than > "fully alphabetic" - we don't request full ordering. What we request is > that groups (xen/, asm/, public/) be ordered within any group, but we > do not (afaia) demand ordering across groups (and indeed commonly we > have asm/ after xen/). Right, but that's **our** coding style. You don't know what's going to be the coding style for other project. Cheers,
On 01.12.2022 12:35, Julien Grall wrote: > On 01/12/2022 11:30, Jan Beulich wrote: >> On 01.12.2022 11:47, Julien Grall wrote: >>> On 01/12/2022 10:44, Juergen Gross wrote: >>>> On 01.12.22 11:12, Julien Grall wrote: >>>>>> 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. >> >> I think so, too. >> >>>> 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. >>> >>> I think Edwin example is a pretty good justification for including >>> stdint.h. >> >> I disagree. The intention has always been for consumers to provide the >> basic C99 types by whatever suitable means they have. Note that in Xen >> we also have no stdint.h. > > I really dislike when I have to find the dependency of an header. This > is really a waste of time... I can see your point, but for Xen's public headers it has always been that way. Plus, as said, adding (unguarded) #include <stdint.h> would even break the building of Xen itself. > If other disagree with that, then the strict minimum would be for this > dependency to be recorded if it hasn't been done (I couldn't find anywhere). It is kind of recorded in xen/include/Makefile, with the three "-include stdint.h" that are used there (of which one probably really ought to be -include cstdint). But I agree this can't really count as documentation. >>> If you have a coding style requiring to order header alphabetically, >>> then the developer may not even be able to include stdint.h without any >>> hackery (e.g. introducing a header that will always be before the Xen >>> public headers). >> >> Just to indicate that commonly style requirements may be weaker than >> "fully alphabetic" - we don't request full ordering. What we request is >> that groups (xen/, asm/, public/) be ordered within any group, but we >> do not (afaia) demand ordering across groups (and indeed commonly we >> have asm/ after xen/). > Right, but that's **our** coding style. You don't know what's going to > be the coding style for other project. Sure, hence me having said "may be" in my reply. Jan
diff --git a/tools/ocaml/.clang-format b/tools/ocaml/.clang-format new file mode 100644 index 0000000000..7ff88ee043 --- /dev/null +++ b/tools/ocaml/.clang-format @@ -0,0 +1,9 @@ +BasedOnStyle: GNU +IndentWidth: 4 + +# override GNU to match Xen ../../CODING_STYLE more closely +AlwaysBreakAfterDefinitionReturnType: None +AlwaysBreakAfterReturnType: None +SpacesInConditionalStatement: true +SpaceBeforeParens: ControlStatements +BreakBeforeBraces: Allman
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. 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. Does not yet reformat any code. No functional change. Signed-off-by: Edwin Török <edvin.torok@citrix.com> --- tools/ocaml/.clang-format | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 tools/ocaml/.clang-format