On 12/11/2019 08:35, Jan Beulich wrote: > On 11.11.2019 21:24, Andrew Cooper wrote: >> --- a/xen/arch/x86/x86_64/mm.c >> +++ b/xen/arch/x86/x86_64/mm.c >> @@ -1077,7 +1077,7 @@ long do_set_segment_base(unsigned int which, unsigned >> long base) >> } >> >> >> -/* Returns TRUE if given descriptor is valid for GDT or LDT. */ >> +/* Returns true if given descriptor is valid for GDT or LDT. */ >> int check_descriptor(const struct domain *dom, seg_desc_t *d) > Wouldn't changes like this one better be accompanied by also adjusting > the return type of the function (there are more examples further down > in common/timer.c)?
No. That is an unrelated change. If I were flush with free time then I might consider doing this and substantially increase the test burden. As it stands, this request is scope creep. > >> --- a/xen/include/asm-arm/arm64/efibind.h >> +++ b/xen/include/asm-arm/arm64/efibind.h >> @@ -107,7 +107,7 @@ typedef uint64_t UINTN; >> #define POST_CODE(_Data) >> >> >> -#define BREAKPOINT() while (TRUE); // Make it hang on Bios[Dbg]32 >> +#define BREAKPOINT() while (true); // Make it hang on Bios[Dbg]32 > You do realize that this and other EFI headers (and perhaps also > ACPI ones) are largely verbatim imports from other projects, > updating of which will become less straightforward by such > replacements? When pulling in the EFI ones I intentionally did not > fiddle with them more than absolutely necessary. Yes, and? It is unacceptable for the acpi headers to forcibly redefine anything in their scope, and its definition of va_args is downright dangerous. All junk like this in header files does nothing but waste space and compiler effort during compilation, and leave people with an slim chance of shooting themselves in the foot. How many times do these get touched? (Rhetorical question. The answer is once (me, clang build fix) since their introduction, 8, 9 and 10 years ago). For the 30s of effort required to tweak once-in-a-blue-moon patches which touch these headers, trimming the junk is a no-brainer. > > If it wasn't for this, I'd have ack-ed the patch despite the other > remark above. > >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -607,7 +607,7 @@ int __must_check donate_page(struct domain *d, struct >> page_info *page, >> #define RAM_TYPE_UNUSABLE 0x00000004 >> #define RAM_TYPE_ACPI 0x00000008 >> #define RAM_TYPE_UNKNOWN 0x00000010 >> -/* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */ >> +/* true if the whole page at @mfn is of the requested RAM type(s) above. */ >> int page_is_ram_type(unsigned long mfn, unsigned long mem_type); > In other comments I already wasn't sure about such replacements, but > let them be. Here, however, you violate coding style by using "true" > instead of "True" (the function returning "int" for now doesn't even > allow the excuse of meaning the identifier rather than the word). Fixed. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
