On 2017-05-18 12:40:30, Laszlo Ersek wrote:
> On 05/18/17 20:49, Jordan Justen wrote:
> > On 2017-05-18 08:14:33, Laszlo Ersek wrote:
> >> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> >> b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> >> index 4247d21d72f8..beb11e3f9a90 100644
> >> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> >> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> >> @@ -58,8 +58,14 @@ typedef struct {
> >> //
> >> // Constants
> >> //
> >> -#define EMU_FVB_BLOCK_SIZE (FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
> >> -#define EMU_FVB_SIZE (2 * FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
> >> +#define EMU_FVB_BLOCK_SIZE \
> >> + EFI_PAGE_SIZE
> >> +#define EMU_FVB_NUM_SPARE_BLOCKS \
> >> + EFI_SIZE_TO_PAGES ((UINTN)FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
> >> +#define EMU_FVB_NUM_TOTAL_BLOCKS \
> >> + (2 * EMU_FVB_NUM_SPARE_BLOCKS)
> >> +#define EMU_FVB_SIZE \
> >> + (EMU_FVB_NUM_TOTAL_BLOCKS * EMU_FVB_BLOCK_SIZE)
> >> #define FTW_WRITE_QUEUE_SIZE \
> >> (FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) - \
> >> sizeof (EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER))
> >
> > In the cases where we don't exceed 80 columns, I don't see the excess
> > newlines as helping here, style-wise.
>
> My first preference would have been
>
> #define SHORT_MACRO_NAME replacement text 1
> #define ANNOYINGLY_LONG_MACRO_NAME replacement text 2
>
> That is, to keep both the macro names and the replacement texts aligned.
> However, that way I wouldn't fit into 80 chars on some lines, and then
> breaking only *some* macro definitions to multiple lines looked
> horrible. Which is why I opted for the current layout: it is uniform,
> and it does preserve the alignment for both macro names and replacement
> texts separately.
I don't think you would make a block of function calls all multiline
if one call required it. I see your point and I agree that aligning
things can be nice if it works out. It seems like it doesn't in this
case.
Could FTW_SPARE_SIZE and FTW_WORKING_SIZE macros help?
If you feel strongly about this current format, then keep it, as I
don't feel too strongly about it.
> >
> > Could you add to the entry-point an assert:
> >
> > ASSERT(FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) %
> > EMU_FVB_BLOCK_SIZE == 0);
>
> Should I squash that into this patch?
Yeah. No need for resend.
-Jordan
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
