Pete: I push your patches into edk2 trunk. Please check them. >-----Original Message----- >From: Pete Batard [mailto:p...@akeo.ie] >Sent: Saturday, March 17, 2018 12:36 AM >To: Gao, Liming <liming....@intel.com>; edk2-devel@lists.01.org >Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017 > >Thanks Liming, much appreciated! > >I'll send the comment harmonization patch as soon as I see the >VS2017/ARM64 changes in edk2 mainline. > >Regards, > >/Pete > >On 2018.03.16 16:31, Gao, Liming wrote: >> 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....@intel.com> >> >> Thanks >> Liming >>> -----Original Message----- >>> From: Pete Batard [mailto:p...@akeo.ie] >>> Sent: Saturday, March 17, 2018 12:12 AM >>> To: edk2-devel@lists.01.org; Gao, Liming <liming....@intel.com> >>> Cc: Ard Biesheuvel <ard.biesheu...@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-boun...@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....@intel.com>; Ard Biesheuvel ><ard.biesheu...@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:p...@akeo.ie] >>>>>>> Sent: Thursday, March 15, 2018 5:28 PM >>>>>>> To: Gao, Liming <liming....@intel.com>; edk2-devel@lists.01.org >>>>>>> Cc: ard.biesheu...@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 >>
_______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
