Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 9158 bytes --]



> On Aug 16, 2019, at 2:40 PM, Vitaly Cheptsov via Groups.Io <vit9696=protonmail.com@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.kinney@intel.com <mailto:michael.d.kinney@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] On Behalf Of Laszlo Ersek
>> > Sent: Friday, August 16, 2019 12:39 PM
>> > To: vit9696@protonmail.com
>> > Cc: devel@edk2.groups.io; leif.lindholm@linaro.org;
>> > afish@apple.com
>> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add
>> > STATIC_ASSERT macro
>> >
>> > On 08/16/19 19:23, vit9696@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
>> >
>> > 
>> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 18959 bytes --]