Laszlo, I am very glad to you for expressing a different opinion as this lets me view the situation from different angles.
I understand your concerns, and believe that most of them should actually be addressed in a way you explain. In fact, I plan to submit more patches myself for everyone's benefit. The exact situation with static assertions is that they are not coming too early, but actually too late. We have been using static assertions in UEFI code for quite some time already, and I believe we are not alone. All of us will benefit from legacy code removal once this patch lands upstream. For your claim that this code is not well tested I should mention that the patch is based on one of the open-source projects I maintain, which everyone can track, and which I believe have gotten reasonable attention from different people with different compilers. For dead code I believe that in EDK II we do not have a good definition for that term as normally done in serious industrial projects like aerospace or military that have no dead code requirement in their SDL. Primarily because EDK II is a library for others to rely on, it is not a self contained system where dead code term is usually defined, standardised and verified against. Whether it is liked or not, the fact EDK II gets continual development is only because different companies, academia, and individuals use its code. I feel bad for these people having to fork, and believe that most value in EDK is what it gives to the outside, not the inside. So supporting a new interface a number of projects use and need makes most sense to me. I do not want to make more changes to core code for multiple reasons as you see above. One of them indeed being some necessary discussion for the use inside EDK II. But I do not believe this a good stopper from giving a working interface to others, which unlike EDK II, actually have defined compilers, infrastructure, and requirements. Hopefully I pointed out to enough reasons to leave you with some doubts and permit this patch to land in as an exception from your personal standpoint. Thank you for understanding and being constructive. Cheers, Vitaly On пт, авг. 16, 2019 at 22:38, Laszlo Ersek <ler...@redhat.com> wrote: > On 08/16/19 19:23, vit9...@protonmail.com wrote: >> Laszlo, >> >> I have already mentioned that the documentation is sufficient as >> _Static_assert is C standard > > Yes, in a release of the ISO C standard that edk2 does not target. > > In addition, edk2 already has several restrictions in place against > standards-conformant code. (Such as bit-shifting of UINT64 values, > initialization of structures with automatic storage duration, structure > assignment, maybe more.) > >> so I do not plan to make a V3 for this patch. > > I find that regrettable. > >> The patch is merge ready. > > Such statements are usually made when people that comment on a patch > arrive at a consensus. The patch may be merge-ready from your > perspective and from Mike's. It is not merge-ready from my perspective. > I hope I'm allowed to comment (constructively) on patches that aren't > strictly aimed at the subsystems I co-maintain. > >> As for usage examples I have an opposing opinion to yours and believe >> it is based on very good reasons. Not using STATIC_ASSERT in the >> current release will make the feature optionally available and let >> people test it in their setups. > > Not using STATIC_ASSERT in the current stable release makes the > STATIC_ASSERT macro definition *dead code* in edk2 proper. I understand > that edk2 is a "kit", and quite explicitly caters to out-of-tree > platforms. That's not a positive trait of edk2 however; it's a negative > one, in my judgement. Whatever we add to the core of edk2, we should > exercise as diligently as we can *inside* of edk2. > >> In case they notice it does not work for them they will have 3 months >> grace period to report it to us and consider making a change. > > That is what the feature freezes are for. The feature is reviewed before > the soft feature freeze, merged (at the latest) during the soft feature > freeze, and bugs can still be fixed during the hard feature freeze. The > community is expected to test diligently during the hard feature freeze. > Perhaps we should extend the hard feature freeze. > > My problem is not that the change is not "in your face". I'm all for > avoiding regressions. My problem is that the code is dead and untestable > without platform changes, even though it could be put to great use in > core code at once. If you think that's too risky, this close to the > stable tag, then one solution is to resubmit at the beginning of the > next development cycle (again with additional patches that utilize the > STATIC_ASSERT macro at once). Developers will then have close to three > months to report and fix issues. > > Another solution would be to conditionally keep VERIFY_SIZE_OF, vs. > using STATIC_ASSERT, for expressing the build-time invariants. The > default would be STATIC_ASSERT. Should it break, people could > immediately switch back to VERIFY_SIZE_OF, without disruption to their > workflows. > > We've done similar things in OvmfPkg in the past. For example: > - USE_LEGACY_ISA_STACK (commit a06810229618 / commit 562688707145), > - USE_OLD_BDS (commit 79c098b6d25d / commit dd43486577b3), > - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit cef83a3050e5). > >> This will also give them 3 months grace period of VERIFY_SIZE_OF macro >> removal in favour of STATIC_ASSERT. Making the change now will let >> people do seamless transition to the new feature and will avoid >> obstacles you are currently trying to create. > > Please stop making claims in bad faith. I'm not trying to "create > obstacles". I'm a fan of STATIC_ASSERT. I'm not a fan of dead code. > >> Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must both be >> separate patchsets with potentially separate BZs. >> >> Thanks for understanding, > > Why are you presenting this as a done deal? The v2 patch was submitted > three days ago, IIUC. > > Also, I wish we could have this discussion without condescension. > > Thanks, > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45863): https://edk2.groups.io/g/devel/message/45863 Mute This Topic: https://groups.io/mt/32850582/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
