[edk2-devel] [PATCH 1/4] OvmfPkg: Update DSC/FDF to use NetworkPkg's include fragment file.

[Laszlo Ersek]

On 05/15/19 09:02, Zhang, Shenglei wrote:
> Hi Laszlo:
> 
>> -----Original Message-----
>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Tuesday, May 14, 2019 9:49 PM
>> To: devel at edk2.groups.io; Zhang, Shenglei <shenglei.zhang at intel.com>
>> Cc: Justen, Jordan L <jordan.l.justen at intel.com>; Ard Biesheuvel
>> <ard.biesheuvel at linaro.org>; Anthony Perard <anthony.perard at citrix.com>;
>> Julien Grall <julien.grall at linaro.org>
>> Subject: Re: [edk2-devel] [PATCH 1/4] OvmfPkg: Update DSC/FDF to use
>> NetworkPkg's include fragment file.

>> (4) Therefore, please *prepend* a patch to this series that eliminates
>> the [LibraryClasses.common.DXE_DRIVER] resolutions altogether, at first.
> 
> Thanks for your comments!
> I'll send a patch first to solve the duplicated library classes about network in
> [LibraryClasses.common.DXE_DRIVER] section.

Thanks for that, I'll review it soon.

>> (6) Please be consistent with the comments that you add near the
>> !include directives. In the current patch, you add comments for the libs
>> and the PCDs, but not for the defines or the components. Please stick
>> with one style -- either add zero comments, or add comments on all of
>> the !includes.
> 
> Actually, for components I add "Network Support" in the comments, which looks like
> aligning with nearby components. Do you thinks it is acceptable?

Sorry, I missed the pre-existent comment there, and the fact that you
were preserving the comment.

So yes, that is fine -- as long as all !include directives uniformly
have a comment (newly added, or inherited from preexistent code), it's OK.

Please note however that your network *defines* !include still lacks a
comment! Please fix that, for consistency with the rest of the !include
directives.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#40644): https://edk2.groups.io/g/devel/message/40644
Mute This Topic: https://groups.io/mt/31616136/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-