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

["Vitaly Cheptsov" <vit9696@protonmail.com>]

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

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.

Best wishes,
Vitaly

On сб, авг. 17, 2019 at 00:04, Kinney, Michael D <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: 8618 bytes --]