Re: [Xen-devel] [PATCH for-4.13] xen: Drop bogus BOOLEAN definitions, TRUE and FALSE

[Jan Beulich <jbeulich@xxxxxxxx>]

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)?

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

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

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel