Add assert to SvPV-ish macros

[khwilliamson]

This is to see how many CPAN modules call these with a differently sized 'len' than the specified 'STRLEN'. See https://github.com/Perl/perl5/issues/19983

[jkeenan]

This is to see how many CPAN modules call these with a differently sized 'len' than the specified 'STRLEN'. See #19983

Can you be a bit more specific as to what your battle plan is here?

For example ... are you proposing to include this in our next monthly development release, such that the volume of CPANtesters failure reports would indicate the scope of the problem?

[khwilliamson]

On 7/24/22 11:08, James E Keenan wrote:

This is to see how many CPAN modules call these with a differently
sized 'len' than the specified 'STRLEN'. See #19983
<https://github.com/Perl/perl5/issues/19983>

Can you be a bit more specific as to what your battle plan is here?

For example ... are you proposing to include this in our next monthly development release, such that the volume of CPANtesters failure reports would indicate the scope of the problem?

That would be one possibility; I hoped that people's comments would indicate if that would be a good idea

[khwilliamson]

On 7/24/22 17:30, Tony Cook wrote:

@.**** commented on this pull request.

---

In sv.h https://github.com/Perl/perl5/pull/19990#discussion_r928326673:

@@ -1902,19 +1902,22 @@ END_EXTERN_C (((SvFLAGS(sv) & (SVf_POK|SVs_GMG)) == SVf_POK) || ((SvFLAGS(sv) & (SVf_IOK|SVp_POK|SVs_GMG)) == (SVf_IOK|SVp_POK)))

#define SvPV_flags(sv, len, flags) \

• (_ASSERT(sizeof(len) == sizeof(STRLEN)) \

Why |_ASSERT()| (which really isn't a name we should define, let alone use) instead of |assert(...),|?

Because it already exists, has since 5.19.7, is documented as API, and has been backported. I don't know why the original has the leading underscores which make it undefined behavior.

We could create a synonym that is legal for us to use. ASSERT_ comes to mind, just to avoid propagating this one further, but the former has a longish history to it.

We can't use plain

assert(),

because on non-DEBUGGING builds, that is a syntax error.

Unfortunately we can't use a static assert here :(

— Reply to this email directly, view it on GitHub https://github.com/Perl/perl5/pull/19990#pullrequestreview-1048699702, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2DH3FTFKTLALFNVTWYN3VVXGXXANCNFSM54P5YHKA. You are receiving this because you authored the thread.Message ID: @.***>

[khwilliamson]

On 7/24/22 11:08, James E Keenan wrote:

This is to see how many CPAN modules call these with a differently
sized 'len' than the specified 'STRLEN'. See #19983
<https://github.com/Perl/perl5/issues/19983>

Can you be a bit more specific as to what your battle plan is here?

For example ... are you proposing to include this in our next monthly development release, such that the volume of CPANtesters failure reports would indicate the scope of the problem?

I think it should go in blead for now, to see what breaks immediately, and then if a bunch does, revert it. If not, then decide about a development release.

— Reply to this email directly, view it on GitHub https://github.com/Perl/perl5/pull/19990#issuecomment-1193358319, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2DH5O57DWY4JDMI6KF6LVVV2CBANCNFSM54P5YHKA. You are receiving this because you authored the thread.Message ID: @.***>

[tonycoz]

We can't use plain assert(), because on non-DEBUGGING builds, that is a syntax error. …

Plain assert(), as used in a comma expression:

(assert(foo),expected result)

has been valid syntax since 2014 when the bug was fixed in 11f9ab1a291.

[khwilliamson]

So what should we do about _ASSERT?

[tonycoz]

So what should we do about __ASSERT_?

I'd suggest documenting it as discouraged, don't use it in new code.

[khwilliamson]

I think we would need to backport your plain assert() fix

[demerphq]

@tonyc I think we should remove all the uses of __ASSERT_ from our code if we don't want it to be used. When I need to create a new macro I look for one that does something similar and crib from it. If all the examples use __ASSERT_ then people will continue to use it. BTW, just for the record could you explain why that spelling/macro is problematic? Are identifiers like that reserved?

[tonyc]

@demerphq I think you pinged the wrong GH user ;)

[khwilliamson]

symbols containing multiple underscores in a row or beginning with an underscore are reserved for the implementation of C itself. There's generally no problem unless our names collide, which is very unlikely. Still, it is better practice to not use those.

I do think C has reserved too large a class.

[khwilliamson]

He meant @tonycoz

[tonycoz]

@tonyc I think we should remove all the uses of __ASSERT_ from our code if we don't want it to be used.

I was mostly considering it unnecessary churn, but removing it avoids technical debt, so removing it really isn't that unnecessary.