Add option 'NoExport' to pp_def #534

jo-37 · 2025-02-17T14:26:44Z

Please excuse me for being so stubborn and tedious. I become obsessed with an idea too easily.

This patch provides an option CallFQ to pp_def that makes function calls in the generated POD's usage section fully qualified.

I was merely trying to find out if I could do that. Then it turned out it was much easier than I expected.

coveralls · 2025-02-17T14:31:04Z

coverage: 66.393%. remained the same
when pulling 3d125f8 on jo-37:contrib
into bda65d4 on PDLPorters:master.

mohawk2 · 2025-02-17T16:37:17Z

I did try to say it was easy! Thank you for making this effort.

However, I can't accept this specific change because it is the incorrect solution to the existing problem. The only correct solution will either:

tell PP what to do (don't export this), so that it will then know to document calls in the auto-generated docs (like Inplace, Lvalue, ....) - that was why I suggested NoExport, which achieves this; or
fix the exports as I have already explained in full detail how to do, thereby rendering the existing docs correct

Any acceptable change to PDL::PP keys will also feature documentation updates. Wanting the docs to be fully right is the point of this exercise, isn't it?

Therefore, please update this so that it meets one of the two criteria above.

jo-37 · 2025-02-17T22:22:35Z

I'm not sure I have a correct understanding of these solutions. Trying to rephrase:

NoExport sounds to me as if a pp_defed function should be exported by default unless NoExport is specified. Wouldn't this break the current behaviour and circumvent pp_add_exported?
We may adjust the documentation such that it matches the implementation or modify the implementation such that it matches the documentation. My interpretation of the second solutions is the latter.

Maybe I am misinterpreting something, but as I understand the alternatives as described above, I dislike both.

mohawk2 · 2025-02-18T00:41:26Z

NoExport sounds to me as if a pp_defed function should be exported by default unless NoExport is specified. Wouldn't this break the current behaviour and circumvent pp_add_exported?

All pp_defed functions are exported by default. If that's not documented, it should be (hint :-). pp_add_exported is only relevant for pure-Perl functions that otherwise don't "exist" and get pp_addpmed with code and docs.

Your preference matters, but isn't directly connected to what happens with the PDL code. Convention and the reality of the last 28ish years of code carries weight and will be considered.

jo-37 · 2025-02-18T13:41:59Z

All pp_defed functions are exported by default.

This is an important detail I had missed and that changes my perception of the first solution completely.

So here is a new attempt in that direction. The key modifications:

Add NoExport as a pp_def option that suppresses the pp_add_exported call and modifies the generated usage examples. Documented in PP.pod.
Use fully qualified function calls in the usage examples if the generated function is not going to be exported unless it overloads a function-like operator of the same name
In Ops.pd: Change the helper routines biop, bifunc, ufunc and cfunc to not export the generated function by default. Here I reversed the behaviour: Export => 1 may be specified - which is the minority here.
Remove the call to pp_export_nothing
The case atan2 in PP.pm: add an optional fourth element noinfix to pp_def's Overload option. This prevents the creation of the erroneous example call $c = $a atan2 $b. (Should I really document this edge case?)

jo-37 · 2025-02-18T20:00:32Z

A somehow related issue:

There is something wrong with the generated usage examples when the ArgOrder option is provided. Affects PDL::Primitive::approx_artol and PDL::MatrixOps::tricpy, where lines are generated with the result as lhs and argument:

$result = approx_artol($got, $expected, $atol, $rtol, $result);
$result = $got->approx_artol($expected, $atol, $rtol, $result);
$C = tricpy($A, $uplo, $C);
$C = $A->tricpy($uplo, $C);

Update: fixed.

mohawk2 · 2025-02-18T21:46:16Z

I will give more specific feedback shortly. But with all these code changes, you haven't added a single test. Therefore, the most we could say is that your changes haven't broken anything currently existing. That is not an acceptable bar to aim for with anything other than the most trivial and/or doc-only changes.

And we can't even say that here, because you've pushed not just one change that fails the CI, but then a second one that fails the CI. That implies the CI results weren't checked even cursorily, because the failures happened within 3 seconds of the push. The failure message is extremely unambiguous as to what is going on (the CI config needed updating), which by a fortunate coincidence I actually did on master quite recently (on the 17th, 9 hours before you opened this PR - it's not clear to me why anyone would open a PR without first checking their repo was fully up to date), so a simple update of master followed by a rebase is all you'd need to fix this.

mohawk2

I think on balance that NoExport needs adding. However, the smaller and better solution to the original problem is to add a NoExport=>1 just to re, im, and the _*abs functions, since that can be used untouched as %extra. That would then allow the removal of the pp_export_nothing.

You've said you "dislike" making the functions near the top of PDL::Ops be exported, but I have no idea why.

lib/PDL/PP.pod

mohawk2 · 2025-02-18T21:56:41Z

lib/PDL/PP.pm

@@ -1504,7 +1507,7 @@ EOF
       } elsif ($argorder) {
         my @allouts = grep $any_out{$_} || $outca{$_}, @args;
         push @argsets, map [[ @inargs[0..$_] ], \@allouts, []],
-           ($#inargs-$noptional)..$#inargs;
+           ($#inargs-$noptional)..$#inargs-1;


You've correctly identified a real problem here. But I believe this is not the right solution, and instead the final index should be calculated based on how many @allouts there are - hardcoding a 1 happens to be correct in the existing cases, but is obviously not correct.

Need to revert this part. Though it looks strange, the original doc seems to be correct:

$result = approx_artol($got, $expected, $atol, $rtol, $result);

describes the actual behavior. A provided argument $result is passed through.

Without ArgOrder the behavior seems to be the same, but the generated doc differs. I think the best I can do here is leave it as-is.

So you're telling me that I'm wrong about ArgOrder (a feature I created), and that actually $result should both be passed in and returned, and that should be documented? Please at least start with the idea that I'm correct about these things.

Here is an example. Two dummy functions that differ in their definition in ArgOrder only, result in basically different generated usages.

use PDL::PP qw(Foo::Bar Foo::Bar foobar); pp_def(foo_1 => Pars => 'a(n); b(n); [o]c(n)', OtherPars => 'int k', ArgOrder => [qw(a b k c)], Doc => 'OtherPars and ArgOrder', ); pp_def(foo_2 => Pars => 'a(n); b(n); [o]c(n)', OtherPars => 'int k', Doc => 'OtherPars only', ); pp_done; unlink 'foobar.xs';

From foobar.pm:

=head2 foo_1 =for sig Signature: (a(n); b(n); [o]c(n); int k) Types: (sbyte byte short ushort long ulong indx ulonglong longlong float double ldouble) =for usage $c = foo_1($a, $b, $k); $c = foo_1($a, $b, $k, $c); # all arguments given $c = $a->foo_1($b, $k); # method call $c = $a->foo_1($b, $k, $c);

=head2 foo_2 =for sig Signature: (a(n); b(n); [o]c(n); int k) Types: (sbyte byte short ushort long ulong indx ulonglong longlong float double ldouble) =for usage $c = foo_2($a, $b, $k); foo_2($a, $b, $c, $k); # all arguments given $c = $a->foo_2($b, $k); # method call $a->foo_2($b, $c, $k);

The modifications I tried so far suppressed any call to foo_1 with $c as argument, which would be a regression.

Still commenting this issue here in this PR before I split it into separate pieces because I'm not sure it will lead to a new PR from my side.

In the ArgOrder case, what needs to happen is when the output is given as an arg, it doesn't get doc-ed as a return value. Achieving that is "a simple matter of programming" (which is often not actually simple).

mohawk2 · 2025-02-18T22:07:40Z

lib/PDL/PP.pm

@@ -1463,7 +1465,8 @@ EOF
       confess "$name error in Overload doc: !=1 output (@outs)" if @outs != 1;
       my @ins = $sig->names_in;
       my @vals = ["\$$outs[0] = ".(
-         !$one_arg ? "\$$ins[0] $op \$$ins[1]" :
+         !$one_arg ?


Conditional operators that are cond1 ? cond2 ? v1 : v2 : v3 are incomprehensible, therefore will not be accepted. You may notice I always use (the comments here are to explain the point, and absolutely not to be added to real code):

cond1 ? v1 : # cond1 gets a "go" at giving the answer cond2 ? v2 : # cond2 gets a "go" at giving the answer v3 # the default when neither condition was true

One of the benefits (added to how it's actually comprehensible) is one can insert "goes" at giving an answer anywhere, and it will be immediately obvious the order of evaluation. As a contributor, it's not instantly obvious how important maintainability (and really, comprehensibility) is, but having maintained this giant codebase for some years, it's now painfully obvious to me :-)

The above points (about conditional operators, and about comprehensibility) should almost certainly be added to a "style" section of the ::DeveloperGuide, and it would be valuable to do so as part of this PR :-)

I never thought about such a systematic use of the conditional operator. I like this approach.

Fixed.

As noted in this latest go - please actually use it :-)

lib/PDL/PP.pm

lib/PDL/Ops.pd

jo-37 · 2025-02-19T08:33:27Z

The failure message is extremely unambiguous as to what is going on (the CI config needed updating), which by a fortunate coincidence I actually did on master quite recently (on the 17th, 9 hours before you opened this PR - it's not clear to me why anyone would open a PR without first checking their repo was fully up to date), so a simple update of master followed by a rebase is all you'd need to fix this.

You fixed the cygwin build, which passes the PR checks. The new failures affect other builds, that probably need to be fixed in devops/github-actions/ci-dist/action.yml. The PR is based on the current top of the master branch.

I'll incorporate your remarks, though this will take some time. Looks like I need to write a completely new test for the generated PODs and I feel this should address not only the changes I made.

jo-37 · 2025-02-19T10:02:04Z

You've said you "dislike" making the functions near the top of PDL::Ops be exported, but I have no idea why.

Primarily a gut feeling, especially when exporting a function that is overloaded. Then I thought "Why not?". The result on an exported sin:

jo@bear:~/Programs/pdl(gen-pod*)$ perl -w -MPDL -E 'say sin(1)'
0.841470984807897
jo@bear:~/Programs/pdl(gen-pod*)$ perl -w -Mblib -MPDL -E 'say sin(1)'
0

So we must not export the overloaded functions. Shall we export as much as we can or not more than required? Aside from my gut feeling I'm indifferent in this question.

jo-37 · 2025-02-19T21:15:35Z

A few notes:

I have no idea why the usage of actions/cache@v2 sometimes fails and sometimes succeeds.
Please let me know what you think about t/pp_pod.t. I'd regard it as a starting point.
A function with multiple output parameters, say a(n); [o]b(n); [o]c(n), gets an example call of foo($a, $b, $c). If this mixes with ArgOrder, this call is not generated. On purpose or is such call not valid?

mohawk2 · 2025-02-28T05:31:29Z

I have no idea why the usage of actions/cache@v2 sometimes fails and sometimes succeeds.

It might be (ironically) a caching issue at GitHub.

Please let me know what you think about t/pp_pod.t. I'd regard it as a starting point.

Tests are important, but this isn't there yet. Please make pp_def return the hash it's made, which can then be looked at in this new test file. This is another benefit of the "usage doc" cases being data structures until the last minute: they can be tested without having to do text parsing.

A function with multiple output parameters, say a(n); [o]b(n); [o]c(n), gets an example call of foo($a, $b, $c). If this mixes with ArgOrder, this call is not generated. On purpose or is such call not valid?

Show me with test cases, because written-out Q&A is far from the best approach.

Please have a look at your commits, adjusting them (e.g. squash, or splitting them up) so that each is a small, useful, final change. The actual chronology of wrestling with these things is not valuable.

There is no reason to document NoExport twice, which you're currently doing.

Please add a test case that proves exporting overloaded functions causes errors. I am surprised and a bit skeptical, but in any case any such error needs to be caught in a test.

jo-37 · 2025-03-01T11:49:35Z

Tried to split the PR into independent pieces, but failed. The changes are too closely related, a split is technically difficult and IMHO not helpful in terms of content.

Therefore I removed the ArgOrder part and squashed the rest.

There is no reason to document NoExport twice, which you're currently doing.

AFAICS everything related to export is documented twice in PP.pod: Once in the regular POD structure and once in the section "Modifying the Symbol Table and Export Behavior". I tried to be consistent.

As I'm not familiar with GitHub's workflows, one stupid question: Who shall resolve/close the review issues you opened?

mohawk2

In this latest form, it's causing a failure in the downstream testing. For a second time, you have updated this pull request, and not checked the CI to see what it says. Is there a reason? It seems to me that is a reasonable expectation.

I will deal with the "separate commits" part in a moment.

mohawk2 · 2025-03-06T02:31:30Z

lib/PDL/PP.pm

         ).";", []
       ];
-       push @vals, [ "$name(\$$in->inplace".(
-           !@args ? '' : ",@{[join ',', map qq{\$$_}, @args]}"
+       my $op = defined($ovl) ? ref($ovl) ? $ovl->[0] : $ovl : '';


The way to do this as the "goes" thing:

!defined($ovl) ? '' : ref($ovl) ? $ovl->[0] : $ovl;

If you have two ? with no : in between, you're not using that style.

lib/PDL/PP.pm

mohawk2 · 2025-03-06T02:43:18Z

lib/PDL/PP.pm

@@ -1463,7 +1465,8 @@ EOF
       confess "$name error in Overload doc: !=1 output (@outs)" if @outs != 1;
       my @ins = $sig->names_in;
       my @vals = ["\$$outs[0] = ".(
-         !$one_arg ? "\$$ins[0] $op \$$ins[1]" :
+         !$one_arg ?


As noted in this latest go - please actually use it :-)

lib/PDL/Ops.pd

mohawk2 · 2025-03-06T02:49:47Z

lib/PDL/PP.pm

@@ -1504,7 +1507,7 @@ EOF
       } elsif ($argorder) {
         my @allouts = grep $any_out{$_} || $outca{$_}, @args;
         push @argsets, map [[ @inargs[0..$_] ], \@allouts, []],
-           ($#inargs-$noptional)..$#inargs;
+           ($#inargs-$noptional)..$#inargs-1;


In the ArgOrder case, what needs to happen is when the output is given as an arg, it doesn't get doc-ed as a return value. Achieving that is "a simple matter of programming" (which is often not actually simple).

mohawk2 · 2025-03-06T03:08:15Z

Tried to split the PR into independent pieces, but failed. The changes are too closely related, a split is technically difficult and IMHO not helpful in terms of content.

The changes can easily be split up, for instance as follows:

a commit that makes PP return the hash-ref, and tests on current behaviour
an additional test for the ArgOrder stuff that constrains it to be correct, and a code fix
a test and fix for the infix stuff
a test and fix for the NoExport stuff

The changes are not at all indivisible. It's not technically difficult, just requires some git branch stuff. There are tutorials. The way I would approach this would be to do a git reset HEAD^, then several git add -p; git commit -m '...' with a git stash and make coretest between each one to make sure that change was working, with a git log -p -w to self-review to make sure I liked it, then git stash pop to repeat the add -p.

A quick note: please don't add whitespace to the UsageDoc (adding a space to the , between arguments).

Therefore I removed the ArgOrder part and squashed the rest.

That is removing one thing that is unequivocally useful to change (since the docs as generated are currently wrong in that case). That's a pity.

There is no reason to document NoExport twice, which you're currently doing.

AFAICS everything related to export is documented twice in PP.pod: Once in the regular POD structure and once in the section "Modifying the Symbol Table and Export Behavior". I tried to be consistent.

Fair enough.

As I'm not familiar with GitHub's workflows, one stupid question: Who shall resolve/close the review issues you opened?

The GitHub interface allows you to "resolve" those things, but please don't since it's the reviewer/maintainer (here, me) who decides whether these things are actually resolved, and it's helpful to them to leave the GH interface showing that.

jo-37 · 2025-03-06T17:58:59Z

Before I start a new refactoring attempt, I have some additional questions and remarks.

it's causing a failure in the downstream testing.

I cannot see any connection between the failing tests in Statistics::Descriptive::PDL and the proposed changes.
What measures shall I take when I encounter a failing downstream test that appears to be unrelated? So far I just waited for the failure to disappear magically.

A quick note: please don't add whitespace to the UsageDoc (adding a space to the , between arguments).

In other cases there is such a space in the UsageDoc. It's just the Inplace part that misses the space. Do you want to keep everything as-is, make Inplace consistent with space or consistent without space (i.e. change the other docs)?

The changes can easily be split up

What shall be the result of the split: Several PRs or a single PR consisting of several commits in the way you described?

Therefore I removed the ArgOrder part and squashed the rest.

That is removing one thing that is unequivocally useful to change

I'll take care of the ArgOrder issue. I didn't want to mix in just another topic in the single commit that it is at the moment.

The GitHub interface allows you to "resolve" those things, but please don't

Actually, I presumed it was your part.

… in Ops.pd

jo-37 · 2025-03-12T12:07:38Z

For the sake of progress I made some assumptions.

The failing downstream test for PDLx::Mask is not related to this PR. The failure just reflects the module's current state in the cpantesters matrix. Ignoring this failure.
Inserting whitespace between arguments for inplace usage docs is the most obvious way to make things consistent.
Several commits within a single PR are useful to guarantee the correct order of their application.

Here is a revised version based on these assumptions.

jo-37 force-pushed the contrib branch from bb98bb0 to bb6c311 Compare February 18, 2025 13:40

jo-37 changed the title ~~Add option 'CallFQ' to pp_def~~ Add option 'NoExport' to pp_def Feb 18, 2025

jo-37 force-pushed the contrib branch from bb6c311 to 44cb9ba Compare February 18, 2025 19:12

mohawk2 requested changes Feb 18, 2025

View reviewed changes

jo-37 force-pushed the contrib branch from 5f7e331 to 08ab735 Compare March 1, 2025 11:45

jo-37 force-pushed the contrib branch from 08ab735 to 326e3ce Compare March 2, 2025 17:01

mohawk2 requested changes Mar 6, 2025

View reviewed changes

jo-37 added 8 commits March 7, 2025 12:11

Add tests for generated UsageDoc

c06d356

Check overloaded functions behave like their CORE counterparts

723c9e1

Add "noinfix" flag to "overload"

16620d4

Suppress infix usage for unary operators

6283c4a

Adjust UsageDoc for "Inplace"

528e949

Fix UsageDoc for "ArgOrder"

e20fa5f

Add "NoExport" to pp_def

8a7034e

Switch from "pp_export_nothing" to "NoExport" and rearrange functions…

3d125f8

… in Ops.pd

jo-37 force-pushed the contrib branch from 326e3ce to 3d125f8 Compare March 12, 2025 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option 'NoExport' to pp_def #534

Add option 'NoExport' to pp_def #534

jo-37 commented Feb 17, 2025

coveralls commented Feb 17, 2025 •

edited

Loading

mohawk2 commented Feb 17, 2025

jo-37 commented Feb 17, 2025

mohawk2 commented Feb 18, 2025 •

edited

Loading

jo-37 commented Feb 18, 2025

jo-37 commented Feb 18, 2025 •

edited

Loading

mohawk2 commented Feb 18, 2025 •

edited

Loading

mohawk2 left a comment

mohawk2 Feb 18, 2025

jo-37 Feb 19, 2025

jo-37 Feb 20, 2025

mohawk2 Feb 28, 2025

jo-37 Feb 28, 2025

mohawk2 Mar 6, 2025

mohawk2 Feb 18, 2025

jo-37 Feb 19, 2025

mohawk2 Mar 6, 2025

jo-37 commented Feb 19, 2025 •

edited

Loading

jo-37 commented Feb 19, 2025

jo-37 commented Feb 19, 2025

mohawk2 commented Feb 28, 2025

jo-37 commented Mar 1, 2025

mohawk2 left a comment

mohawk2 Mar 6, 2025

mohawk2 Mar 6, 2025

mohawk2 Mar 6, 2025

mohawk2 commented Mar 6, 2025

jo-37 commented Mar 6, 2025

jo-37 commented Mar 12, 2025 •

edited

Loading

Add option 'NoExport' to pp_def #534

Are you sure you want to change the base?

Add option 'NoExport' to pp_def #534

Conversation

jo-37 commented Feb 17, 2025

coveralls commented Feb 17, 2025 • edited Loading

mohawk2 commented Feb 17, 2025

jo-37 commented Feb 17, 2025

mohawk2 commented Feb 18, 2025 • edited Loading

jo-37 commented Feb 18, 2025

jo-37 commented Feb 18, 2025 • edited Loading

mohawk2 commented Feb 18, 2025 • edited Loading

mohawk2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jo-37 commented Feb 19, 2025 • edited Loading

jo-37 commented Feb 19, 2025

jo-37 commented Feb 19, 2025

mohawk2 commented Feb 28, 2025

jo-37 commented Mar 1, 2025

mohawk2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohawk2 commented Mar 6, 2025

jo-37 commented Mar 6, 2025

jo-37 commented Mar 12, 2025 • edited Loading

coveralls commented Feb 17, 2025 •

edited

Loading

mohawk2 commented Feb 18, 2025 •

edited

Loading

jo-37 commented Feb 18, 2025 •

edited

Loading

mohawk2 commented Feb 18, 2025 •

edited

Loading

jo-37 commented Feb 19, 2025 •

edited

Loading

jo-37 commented Mar 12, 2025 •

edited

Loading