Re: [PATCH 0/4] Add ARM64 support for VS2017

[Pete Batard <pete@akeo.ie>]

I understand where you're coming from, but that means I have to recreate 
this patch set, and then create a new patch for the .S (because it makes 
zero sense to require the same comment style on the .asm and not request 
a follow through for the .S).

My time being limited, I'd rather only have to produce one new patch, 
that will harmonize the comments for both .S and .asm at the same time.

The end result will be exactly the same, so I'm going to have to insist 
that we split the comment harmonization (which is a very minor issue and 
should hardly be seen as a showstopper for the patch series in the first 
place) into a subsequent patch.

Thank you,

/Pete

On 2018.03.16 15:56, Gao, Liming wrote:
> Pete:
>    I understand the existing .S file has the inconsistent comment style. I also know new added ASM files are converted from .S files. But, my comment is for this patch that adds new ASM files. I expect new added ASM files have the same style. If you check ARM arch ASM files, you will find they all have the same style.
> 
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Pete Batard
>> Sent: Friday, March 16, 2018 7:04 PM
>> To: edk2-devel@lists.01.org
>> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>
>> On 2018.03.16 08:24, Gao, Liming wrote:
>>> Pete:
>>>      .S for GCC assembly, .asm for MSFT assembly. They can have the different comment style.
>>
>> Yes, but as I explained, the actual original issue is that our current
>> .S files do *not* have the same comment styles in the first place.
>>
>> If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll see
>> that is uses '//' for comments, whereas other .S files, such as
>> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
>>
>> So that is our actual issue here: Regardless of VS2017, the GCC assembly
>> files for AARCH64 we currently have do not use the same comment style.
>>
>> Thus, the only reason why the .asm don't have the same comment style in
>> our proposal is because the .S, which we derived the .asm from, don't.
>> This means that either we should fix the .S too, or we shouldn't fix
>> anything at all.
>>
>>>     Here, my comment is to make sure .asm files have the same comment style. I don't request to change .S file.
>>
>> And what I am saying is that it makes little sense to harmonize the
>> comment style for the .asm files, if we're not going to do the same for
>> the .S files as well. It just doesn't seem fair in my book to have the
>> VS2017 assembly files held to a higher standard than the GCC ones. So
>> either we need to fix both, or we fix none at all.
>>
>> But as I indicated in my last e-mail, I am planning to send an
>> additional patch that does comment harmonization, for both .S and .asm,
>> *after* this VS2017 series has been applied to mainline. So the change
>> you request will happen. Just not as part of this patch series.
>>
>> And the reason I have insist on splitting these changes is because, if
>> we have to alter the .S files to be consistent, then this comment
>> harmonization request should logically be handled separately from the
>> VS2017 effort.
>>
>> Please let me know if you still think having a future separate patch,
>> that will do .S and .asm comment harmonization, does not make sense.
>>
>> Regards,
>>
>> /Pete
>>
>>>
>>>> -----Original Message-----
>>>> From: Pete Batard [mailto:pete@akeo.ie]
>>>> Sent: Thursday, March 15, 2018 5:28 PM
>>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>>>> Cc: ard.biesheuvel@linaro.org
>>>> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
>>>>
>>>> Hi Liming,
>>>>
>>>> Thanks for reviewing the patches.
>>>>
>>>> On 2018.03.15 06:15, Gao, Liming wrote:
>>>>> Pete:
>>>>>      For new added ASM file in BaseLib, could you use the same comment style
>>>>> for them? ASM use ; for the comment. Most of new files uses ; as the
>>>>> comment, but switchstack is not.
>>>>
>>>> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
>>>> assembler specifics removed, and MSVC assembler specifics added.
>>>>
>>>> I did not change the comment style from the original files, so the real
>>>> issue here is that our GCC assembly files for AARCH64 do not use the
>>>> same comment style.
>>>>
>>>> I'm fine with trying to harmonize the comment styles, but seeing as this
>>>> needs to be done for both the .S and .asm, I'd rather send a patch to do
>>>> that *after* these VS2017 changes have been applied, as I don't consider
>>>> this correction to in scope of this patch series (because logically, the
>>>> introduction of VS2017 should not alter any of the .S files, unless we
>>>> reuse them, which we don't).
>>>>
>>>> If you agree to apply this series, I'll make sure to send a non
>>>> VS2017-specific additional patch, that does what you request for both
>>>> the .S and .asm.
>>>>
>>>>> Besides, compared to Arm arch assembly
>>>>> file, I don't find CpuPause.asm. Is it required?
>>>>
>>>> That file doesn't exist for GCC (as you will see there is no
>>>> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have one for
>>>> VS2017 either.
>>>>
>>>> Regards,
>>>>
>>>> /Pete
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel