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

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

Yes. This is a minor issue. So, I think the effort is small. If it is a big task to you, you can separate it into another patch. 

And, I don't expect this minor issue break your patches. I give my Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> -----Original Message-----
> From: Pete Batard [mailto:pete@akeo.ie]
> Sent: Saturday, March 17, 2018 12:12 AM
> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
> 
> 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