Add assert to SvPV-ish macros #19990
Conversation
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? |
|
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
<#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
|
| @@ -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)) \ | |||
There was a problem hiding this comment.
Why __ASSERT_() (which really isn't a name we should define, let alone use) instead of assert(...),?
Unfortunately we can't use a static assert here :(
There was a problem hiding this comment.
Bram++ pointed me to a discussion about using a static assert expression. I had a go at that, and am stymied.
Yes, one can create such a thing for C, but apparently not for C++, without using lambda expressions. But I don't consider this a show stopper. For C++ compilations, just expand to nothing
The problem I run into is many compilation warnings because we enable C++ compat warnings. AFAICT the pragmas to turn those off around these are statements, not an expression
|
On 7/24/22 17:30, Tony Cook wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In sv.h <#19990 (comment)>:
> @@ -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
<#19990 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2DH3FTFKTLALFNVTWYN3VVXGXXANCNFSM54P5YHKA>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
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
<#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
<#19990 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA2DH5O57DWY4JDMI6KF6LVVV2CBANCNFSM54P5YHKA>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
So what should we do about _ASSERT? |
I'd suggest documenting it as discouraged, don't use it in new code. |
|
I think we would need to backport your plain assert() fix |
|
@tonyc I think we should remove all the uses of |
|
@demerphq I think you pinged the wrong GH user ;) |
|
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. |
|
He meant @tonycoz |
I was mostly considering it unnecessary churn, but removing it avoids technical debt, so removing it really isn't that unnecessary. |
This is to see how many CPAN modules call these with a differently sized 'len' than the specified 'STRLEN'. See Perl#19983
|
@khwilliamson this p.r. has had no activity or discussion since December 2022. My impression is that people were not enthusiastic about it. Can we close it? |
jkeenan
added
the
Closable?
This is to see how many CPAN modules call these with a differently sized
'len' than the specified 'STRLEN'. See
#19983