Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promote std.process.Config.preExecFunction to a delegate #8989

Merged

Conversation

zopsicle
Copy link
Contributor

std.process.Config.preExecFunction is now a delegate instead of a function pointer, and can therefore capture an environment, for example:

import core.sys.linux.sys.prctl : PR_SET_PDEATHSIG, prctl;
import std.process : Config, execute;

void runProgram(int pdeathsig)
{
    execute(
        ["program"],
        config: Config(
            preExecFunction: () @trusted =>
                prctl(PR_SET_PDEATHSIG, pdeathsig, 0, 0, 0) != -1,
        ),
    );
}

Despite function pointers implicitly converting to delegates, this is a backwards-incompatible change, as user code may rely on the field being a function pointer. For example, code like the following will no longer compile:

import std.process : Config;

nothrow pure @nogc @safe
bool f() { return true; }

void example()
{
    auto config = Config(preExecFunction: &f);
    bool function() g = config.preExecFunction;
}

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @chloekek! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8989"

std.process.Config.preExecFunction is now a delegate instead of a function
pointer, and can therefore capture an environment, for example:

    import core.sys.linux.sys.prctl : PR_SET_PDEATHSIG, prctl;
    import std.process : Config, execute;

    void runProgram(int pdeathsig)
    {
        execute(
            ["program"],
            config: Config(
                preExecFunction: () @trusted =>
                    prctl(PR_SET_PDEATHSIG, pdeathsig, 0, 0, 0) != -1,
            ),
        );
    }

Despite function pointers implicitly converting to delegates, this is a
backwards-incompatible change, as user code may rely on the field being a
function pointer. For example, code like the following will no longer compile:

    import std.process : Config;

    nothrow pure @nogc @safe
    bool f() { return true; }

    void example()
    {
        auto config = Config(preExecFunction: &f);
        bool function() g = config.preExecFunction;
    }
@zopsicle zopsicle force-pushed the std.process.Config.preExecFunction-delegate branch from 8e850cc to 5ce5344 Compare April 25, 2024 23:14
@schveiguy
Copy link
Member

Despite function pointers implicitly converting to delegates

This is not true, you cannot assign a function pointer to a delegate.

If you are making a function literal, then yes, the compiler can make it a delegate (with a null context pointer). Otherwise, you need to use the function std.functional.toDelegate.

@dkorpel
Copy link
Contributor

dkorpel commented Apr 26, 2024

For Phobos v2, we want to avoid breaking changes, so I suggest keeping preExecFunction and adding preExecDelegate. It could be an error if both are set at the same time.

@CyberShadow
Copy link
Member

How about making it a delegate, and adding a setter taking a function and converting it to a delegate using std.functional.toDelegate?

@zopsicle
Copy link
Contributor Author

@dkorpel How about calling both if both are set?

@CyberShadow There would also be a getter, what would it do?

@dkorpel
Copy link
Contributor

dkorpel commented Apr 26, 2024

How about calling both if both are set?

That also works

@LightBender
Copy link
Contributor

@chloekek Phobos 3 is under active development right now, that would be the ideal time to make the name change. We're not working on the process module yet, so we'll need to remember it for later.

@CyberShadow
Copy link
Member

CyberShadow commented Apr 26, 2024

@CyberShadow There would also be a getter, what would it do?

I am struggling to think of a use case where you will want to get the function pointer that you placed in std.process.Config. It might not be worth worrying about.

std.process.Config.preExecDelegate is just like
std.process.Config.preExecFunction, but can capture an environment, for
example:

    import core.sys.linux.sys.prctl : PR_SET_PDEATHSIG, prctl;
    import std.process : Config, execute;

    void runProgram(int pdeathsig)
    {
        execute(
            ["program"],
            config: Config(
                preExecDelegate: () @trusted =>
                    prctl(PR_SET_PDEATHSIG, pdeathsig, 0, 0, 0) != -1,
            ),
        );
    }

preExecFunction is retained for backwards compatibility. If both
preExecFunction and preExecDelegate are given, both are called.
@zopsicle
Copy link
Contributor Author

I added preExecDelegate as a separate field in the latest commit. If both
preExecFunction and preExecDelegate are given, both are called.

@dkorpel dkorpel merged commit 8e6f772 into dlang:master Apr 28, 2024
10 checks passed
@zopsicle zopsicle deleted the std.process.Config.preExecFunction-delegate branch April 29, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants