Re: [edk2-devel] [PATCH 1/1] ArmVirtPkg: Remove meaningless comment

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 7/6/21 5:57 PM, Laszlo Ersek wrote:
> On 07/06/21 11:49, Philippe Mathieu-Daudé wrote:
>> From: Philippe Mathieu-Daude <philmd@redhat.com>
>>
>> The "Shell Embedded Boot Loader" description (added in
>> commit 6f5872b1f401) does not add any value, remove it.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Cc: Julien Grall <julien@xen.org>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc           | 2 +-
>>  ArmVirtPkg/ArmVirtKvmTool.fdf        | 2 +-
>>  ArmVirtPkg/ArmVirtXen.fdf            | 2 +-
>>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 2 +-
>>  4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index d9abadbe708c..e17238e63803 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -376,7 +376,7 @@ [Components.common]
>>    MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
>>  
>>    #
>> -  # UEFI application (Shell Embedded Boot Loader)
>> +  # UEFI application
>>    #
>>    ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf {
>>      <PcdsFixedAtBuild>
>> diff --git a/ArmVirtPkg/ArmVirtKvmTool.fdf b/ArmVirtPkg/ArmVirtKvmTool.fdf
>> index 076155199905..8ad67233dc90 100644
>> --- a/ArmVirtPkg/ArmVirtKvmTool.fdf
>> +++ b/ArmVirtPkg/ArmVirtKvmTool.fdf
>> @@ -174,7 +174,7 @@ [FV.FvMain]
>>    INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
>>  
>>    #
>> -  # UEFI application (Shell Embedded Boot Loader)
>> +  # UEFI application
>>    #
>>    INF ShellPkg/Application/Shell/Shell.inf
>>    INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>> diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
>> index 8fbbc2313aff..bab4af446cfb 100644
>> --- a/ArmVirtPkg/ArmVirtXen.fdf
>> +++ b/ArmVirtPkg/ArmVirtXen.fdf
>> @@ -178,7 +178,7 @@ [FV.FvMain]
>>    INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
>>  
>>    #
>> -  # UEFI application (Shell Embedded Boot Loader)
>> +  # UEFI application
>>    #
>>    INF ShellPkg/Application/Shell/Shell.inf
>>    INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
>> index 5b1d10057545..5ecde9233951 100644
>> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
>> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
>> @@ -100,7 +100,7 @@ [FV.FvMain]
>>    INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
>>  
>>    #
>> -  # UEFI application (Shell Embedded Boot Loader)
>> +  # UEFI application
>>    #
>>    INF ShellPkg/Application/Shell/Shell.inf
>>    INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>>
> 
> I'm really sorry Phil, but even the new comment looks just super weird
> to me. I've grepped the edk2 codebase for
> "ShellPkg/Application/Shell/Shell.inf", and looked at the leading
> context (approx. 30 lines) near every match -- and now I actually think
> we don't need *any* comments here.

Yes I agree.

> I mean we could say "UEFI Shell
> Application", but is that really helpful? I have no idea.
> 
> I'll let other ArmVirtPkg reviewers comment on this.
> 
> Thanks
> Laszlo
>