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

["Gao, Liming" <liming.gao@intel.com>]

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