On Thu, Oct 29, 2020 at 04:51:37PM -0500, Glenn Washburn wrote: > On Tue, 27 Oct 2020 19:03:36 +0100 > Daniel Kiper <dki...@net-space.pl> wrote: > > On Mon, Oct 19, 2020 at 06:09:55PM -0500, Glenn Washburn wrote:
[...] > > > diff --git a/include/grub/types.h b/include/grub/types.h > > > index 035a4b528..8b4267ebd 100644 > > > --- a/include/grub/types.h > > > +++ b/include/grub/types.h > > > @@ -319,4 +319,7 @@ static inline void grub_set_unaligned64 (void > > > *ptr, grub_uint64_t val) > > > > > > #define GRUB_CHAR_BIT 8 > > > > I think we should not change the name of this constant. It seems to > > me that it was named in similar way to C/gcc/clang CHAR_BIT and/or > > __CHAR_BIT__. However, I think we should change its definition. It > > should be > > > > #define GRUB_CHAR_BIT __CHAR_BIT__ > > I can add that into this commit. It has to be separate commit because it is logically different change. > > Additionally, the build should stop if __CHAR_BIT__ is not defined nor > > GRUB_CHAR_BIT is not equal 8. > > I'm not sure exactly how you want that done. #ifndef __CHAR_BIT__ #error __CHAR_BIT__ is not defined #else __CHAR_BIT__ != 8 #error __CHAR_BIT__ is not equal 8 #else #define GRUB_CHAR_BIT __CHAR_BIT__ #endif > > Last but not least, the GRUB_CHAR_BIT definition should be right > > behind includes in the include/grub/types.h. > > > > > +#define GRUB_TYPE_BIT(type) (sizeof(type) * GRUB_CHAR_BIT) > > > > I agree with Patrick, it should be named GRUB_TYPE_BITS. > > Patrick says my logic makes sense for him. I can still change it, yes? Ugh... I missed his last email somehow. Sorry about that. I still think it should be GRUB_TYPE_BITS(). So, please rename GRUB_TYPE_BIT() to GRUB_TYPE_BITS(). GRUB_CHAR_BIT name should stay as is. > > > +#define GRUB_TYPE_MAX(type) ((2 * ((1ULL << (GRUB_TYPE_BIT(type) - > > > 1)) - 1)) + 1) > > > > This only works for unsigned types. So, I think we should have three > > (four?) macros here: (GRUB_TYPE_U_MIN? (which is obviously 0)), > > GRUB_TYPE_U_MAX, GRUB_TYPE_S_MIN and GRUB_TYPE_S_MAX. > > How about I just rename GRUB_TYPE_MAX to GRUB_TYPE_U_MAX and someone > (you?) can add in the rest? I'm not sure how I'd do the signed > variants that works across all architectures. I am not going to insist on adding GRUB_TYPE_S_MIN/GRUB_TYPE_S_MAX if you think it is complicated. Though please rename GRUB_TYPE_MAX to GRUB_TYPE_U_MAX. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
