On Thu, 15 Dec 2022 at 01:04, Michael Kubacki <mikub...@linux.microsoft.com> wrote: > > I'm just trying to understand your position. > > Are you saying you would rather people check in typos and then later > have patches come into the package to fix them? > > For example, like these: > > - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021 > - ArmPkg: https://edk2.groups.io/g/devel/message/97022 > > Why not just have the code checked in without typos in the first place? > > Checking in typos creates more review work, makes the code history have > typo fix patches that could be avoided, and impacts accessibility. > > I spend far more time with edk2 overhead such as email formatting > problems, keeping track of maintainer email address changes when > updating patches, mapping email replies back to code, and so on that > does not improve the code. A spell checker only takes seconds and is > built into edk2 CI. >
SpellCheck cannot distinguish between typos and valid uses of non-english words. ArmPkg is not made up of english prose, it is unidiomatic even for EDK2 as its code contains many of the hundreds of mnemonics and acronyms that are defined in the ARM Architecture Reference Manual. With SpellCheck enabled, we need to extend the exception list every time the use of such a word gets introduced into the code, which is tedious and pointless, given that the exception list is already hundreds of entries long, and you are even adding *known typos* to it. Whether or not a patch passes SpellCheck is therefore becoming a poor measure of quality. And fundamentally, given that the resulting object code is identical, SpellCheck does not contribute *at all* to making the code that actually gets shipped any better. I do care about typos, and if this wasn't so disruptive to my normal development process, I wouldn't object to it, but getting a simple PR merged has already become a massive waste of time due to the rigid uncrustify rules (which I am not as excited about either, tbh). So rejecting PRs simply because some word does not appear in the list is not acceptable to me. Unless I get a button in the GitHub UI that permits me to force-merge a PR if it failed the SpellCheck pass. Or adds it to the exception list automatically. I would also like to point out that all this focus on code aesthetics that do not contribute to the object code at all is distracting from things that would actually improve the safety and robustness of the code. During my time at ARM, I implemented a prototype for IBT/BTI in EFI, which -given how much EFI relies on function pointers and indirect calls- would be a very useful thing to have, both on x86 and arm64. Unfortunately, this requires a PE/COFF spec change, so that .text sections can be marked as having been built with brach-tracking landing pads. At the time, we tried various contacts in Microsoft to find the people that could help author such a change to the PE/COFF spec, but I never got anywhere. So if you are interested, and want to improve EDK2 at a level that actually makes the shipping code more robust, we could collaborate on this (PE spec change, UEFI spec change, EDK2 and Linux changes both for x86 and arm64[0]) so that firmware runs with indirect branch protection enabled when the hardware supports it. [0] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-bti -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97433): https://edk2.groups.io/g/devel/message/97433 Mute This Topic: https://groups.io/mt/95678218/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
