Andrew, Mike, Actually thanks for making me recheck it. I have just installed doxygen, and can confirm that it can generate parameters for non-functional macros. This was my main reason for going with /// comment style, which is used for all other plain macros. I have just submitted V3 version with this fixed.
With C language modernisation I fully agree that it needs to be done reasonably, and that is why I also try to be very realistic on what people actually use. For instance, one of the issues that caused several problems is the use of 1-sized arrays instead of flexible array members. A couple years ago that was still there just because there still existed some compilers that did not support [] syntax. Best wishes, Vitaly > 17 авг. 2019 г., в 1:58, Andrew Fish <af...@apple.com> написал(а): > > > >> On Aug 16, 2019, at 2:40 PM, Vitaly Cheptsov via Groups.Io >> <vit9696=protonmail....@groups.io <mailto:vit9696=protonmail....@groups.io>> >> wrote: >> >> Mike, >> >> I missed your message while writing mine, but I am afraid I disagree with >> the functional macro usage for this feature. >> >> I explicitly quoted C standard static_assert definition in one of my >> previous messages, and I want EDK II to be as close to standard C as >> possible. >> >> This will avoid a lot of confusion for newcomers and will let us later adopt >> a more flexible single and double argument interface when it gets >> standardised. >> >> For these reasons altogether, I am not positive the macro could get a >> doxygen documentation as it is not designed to have any arguments in the >> first place. >> > > Vitaly, > > I don't understand your logic? It is always possible to write a comment in C > code? > > In terms of the C standard and the EFI type system we kind of have a long > history of how the code ended up like it is. The (U)EFI spec defined its own > type system (and ABI) as a way of specifying interoperability so the code got > built on top of that. 20 years ago we shied away from having and EFI code > base produce definitions of standard C things as we wanted to make it easier > to import chunks of code in OS loaders that needed to get ported to EFI from > lots of different vendors. Also one of the early compilers that we > standardized on was VC2003 and that does not even fully support C99. For some > reason it seems VC2008 was also a popular target for some time. I don't think > VC++ got around to C99 until 2013/2015. So that is kind how the edk2 ended up > with its own type system. > > I'm all for modernization of the C code as long we are thoughtful about > compatibility. For example I still see that VS2008 is a supported > BaseTools/Conf/tools_def.template. > > Thanks, > > Andrew Fish > > >> Best wishes, >> Vitaly >> >> On сб, авг. 17, 2019 at 00:04, Kinney, Michael D <michael.d.kin...@intel.com >> <mailto:michael.d.kin...@intel.com>> wrote: >>> >>> Laszlo, >>> >>> I agree that better comments/documentation of STATIC_ASSERT() >>> for EDK II usages is required. For example, EDK II defines >>> the ASSERT() macro which is based on the standard C function >>> assert(), but we still document it fully for EDK II usage. >>> >>> /** >>> Macro that calls DebugAssert() if an expression evaluates to FALSE. >>> >>> If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED >>> bit of PcdDebugProperyMask is set, then this macro evaluates the Boolean >>> expression specified by Expression. If Expression evaluates to FALSE, then >>> DebugAssert() is called passing in the source filename, source line number, >>> and Expression. >>> >>> @param Expression Boolean expression. >>> >>> **/ >>> #if !defined(MDEPKG_NDEBUG) >>> #define ASSERT(Expression) \ >>> do { \ >>> if (DebugAssertEnabled ()) { \ >>> if (!(Expression)) { \ >>> _ASSERT (Expression); \ >>> ANALYZER_UNREACHABLE (); \ >>> } \ >>> } \ >>> } while (FALSE) >>> #else >>> #define ASSERT(Expression) >>> #endif >>> >>> I would like to see the macro documentation for >>> STATIC_ASSERT() with the Doxygen style description of the >>> parameters. The fact I asked if STATIC_ASSERT() supported >>> the 2nd message parameter should have been a trigger >>> for me to ask for the more complete macro comment block. >>> The fact that this macro can be directly mapped to >>> built in compiler name makes the implementation simple, >>> but other implementations are possible for compilers >>> that do not support this feature directly. This is why >>> the complete description of the EDK II version is required. >>> >>> I would like to see a V3 with the complete description. >>> >>> In general, I agree it is better if there is code that >>> uses a new feature in the code base, so the feature can >>> be tested. A second option we can consider going forward >>> is for unit test code to be submitted with a new feature, >>> so even if there are no consumers from the EDK II repos, >>> the feature can still be tested as the EDK II repos are >>> maintained. A third option is for community members to >>> provide Tested-by responses to the feature along with >>> statements in the Bugzilla that clearly documents how the >>> the feature was tested. >>> >>> Best regards, >>> >>> Mike >>> >>> > -----Original Message----- >>> > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> >>> > [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of >>> > Laszlo Ersek >>> > Sent: Friday, August 16, 2019 12:39 PM >>> > To: vit9...@protonmail.com <mailto:vit9...@protonmail.com> >>> > Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; >>> > leif.lindh...@linaro.org <mailto:leif.lindh...@linaro.org>; >>> > af...@apple.com <mailto:af...@apple.com> >>> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> > STATIC_ASSERT macro >>> > >>> > On 08/16/19 19:23, vit9...@protonmail.com <mailto: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 (#45869): https://edk2.groups.io/g/devel/message/45869 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] -=-=-=-=-=-=-=-=-=-=-=-
signature.asc
Description: OpenPGP digital signature
