Add ability to specify assert() calls in embed.fnc

[khwilliamson]

This allows the removal of the G flag, and the removal of three customized PERL_ARGS_ASSERT macros. One of these showed the brittleness of the old scheme, as after it was written, code was added to improve the generic versions of these macros, but no one realized to update the custom one. This can no longer happen with this pull request

This commit also improves the error checking of embed.fnc entries; adds comments and white space

• This set of changes includes a perldelta entry

[khwilliamson]

In addition to the requested changes, I added a \b to the matching of assert( in HeaderParser, so that it won't match something just ending in assert

And I added, moved some comments in embed.fnc about this new feature

[leonerd]

These changes look good, but it'd be useful to squash the commits down a bit. We don't need all 11 distinct ones.

Maybe one commit to add the new assert ability, one to swap the existing uses of the G flag and custom macros, and one final one to remove support for that G flag.

[bram-perl]

These changes look good, but it'd be useful to squash the commits down a bit. We don't need all 11 distinct ones.

What I would personally do is merge it with a merge-commit so that it's clear it's one block of related changes.
But that does not appear to be something that is typically done in blead. (But it does happen. Also not sure if the GitHub UI allows it and/or makes it easy or hard.)

[jkeenan]

These changes look good, but it'd be useful to squash the commits down a bit. We don't need all 11 distinct ones.

I would appreciate some squashing along the lines suggested by @leonerd because it would reduce slightly the number of commits that need to be traversed in a bisection used to diagnose bugs in the future.

What I would personally do is merge it with a merge-commit so that it's clear it's one block of related changes. But that does not appear to be something that is typically done in blead.

$ git log --format=fuller --date=short --since 2024-08-07 --reverse | grep -c '^commit '
2048
$ git log --format=fuller --date=short --since 2024-08-07 --reverse | grep -c '^Merge: '
14

Fewer than 1% of all commits in the last 365 days -- but almost all of them were the culmination of projects long in development and were performed by major contributors. Even within such merges, however, some squashing can be beneficial.

[bram-perl]

These changes look good, but it'd be useful to squash the commits down a bit. We don't need all 11 distinct ones.

I would appreciate some squashing along the lines suggested by @leonerd because it would reduce slightly the number of commits that need to be traversed in a bisection used to diagnose bugs in the future.

It would not make any significant different on the number of bisect runs. (It's a binary search: it reduces the search space by half each time. So bisecting 900 commits should take 10 builds. Bisecting 1000 commits should also take 10 builds. (2 ** 10 =1024)).

Having clear, small and obvious commits with clear explanation will make viewing history a whole lot easier. (And it will help when you do end up with the bisect on a small commit rather the one large commit.)

I went through the commits and personally I wouldn't squash any of them. I find all of them split reasonably well.

What I would personally do is merge it with a merge-commit so that it's clear it's one block of related changes. But that does not appear to be something that is typically done in blead.

$ git log --format=fuller --date=short --since 2024-08-07 --reverse | grep -c '^commit '
2048
$ git log --format=fuller --date=short --since 2024-08-07 --reverse | grep -c '^Merge: '
14

Fewer than 1% of all commits in the last 365 days

Because the option is disabled in the GitHub UI. Not because it wouldn't be handy - at least in my opinion.
Just look at the recent history: a lot of commits that start with the exact same prefix. To me that is a sign that a merge-commit would be of use because all these changes are very likely related.

[khwilliamson]

I've done a few bisections recently. We're at 40K commits when the bisect kicks in. We don't need an extra step until we reach 65536 commits. That's more than half again as many as we've ever done. That means that trying to conserve commits won't help bisecting.

Anything we can do to save human time helps us. Bite-sized commits are helpful in this regard.

[bram-perl]

Commit regen/embed.pl: Improve error checking

This commit needs rechecking;.

Same commit in the previous push: a281942
--> it removed the block on line 259-265:

-                  if (   $args_assert_line
-                        && $arg =~ /\*/
-                        && $arg !~ /\b(NN|NULLOK)\b/ )
-                    {
-                        warn "$func: $arg needs NN or NULLOK\n";
-                        ++$unflagged_pointers;
-                    }
-

In this commit it's kept..

I believe the block on lines 259-265 should be removed because the same check is now done here.

[khwilliamson]

This was a rebase -i error. I'm not sure if it was me or rere. Both possibilities are scary (that I missed it)

[bram-perl]

Commit embed.fnc: Add ability to customize ARGS_ASSERT macros

Just a side note: this is a value slice of a hash ref, it can also be written using postderef as:

my ($flags, $retval, $plain_func, $args, $assertions ) = $embed->@{qw(flags return_type name args assertions)};

(Not asking for a change; just offering it as hint, feel free to ignore it.)

[khwilliamson]

These lines at the beginning explain why not:

require 5.004;  # keep this compatible, an old perl is all we may have before
                # we build the new one

[khwilliamson]

(That's not to say that that dictum has been obeyed in all commits; and maybe the number should be changed to more recent.)

[bram-perl]

In that case you should update the line:

        push @asserts, $assertions->@* if $assertions;

which you added since that too is using post-deref.

[khwilliamson]

Yeah; I imagine there are others that have crept in.

[khwilliamson]

I have now added a perldelta entry

[khwilliamson]

@leonerd , Both @bram-perl and I think these commits are a reasonable minimal set. But the series should be added as a merge commit so that they can be considered as a whole, as is our policy in perlgit. I have filed #23547 to either make that easier or to make it harder to do a "Rebase and merge" from GH for series of commits.

[khwilliamson]

My rules for what needs to be separate commits, is "Would it ever be better to keep this change if we needed to revert the rest?" And sometimes for indentation changes, "Does this change make another change's difference listing easier enough to grok to be worth it?"