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

druntime: replaces top level version(..) branching by tags #24

Closed
wants to merge 31 commits into from

Conversation

denizzzka
Copy link
Contributor

@denizzzka denizzzka commented Jan 12, 2024

@adamdruppe
Copy link
Contributor

soooo i like the idea of replacing version... but i don't really like this approach. (and not just cuz it would break the build on my computer lol)

I tried to do something similiar with simpledisplay back in the day and i regret it. so im simpledisplay, there's a top-level interface and then a platform mixin. so like adapting the idea here it would look like

module core.sync.event;

import core.sync.platform.event_impl; // placeholder name

struct Event {
   void set() { impl.set(); }
   void reset() { impl.reset(); }
   // yada yada yada

   mixin EventImpl impl;
}

And then the impl thing defines mixin templates with the platform specifics. (You don't even strictly need to define the names top level, since the impl can mix them in too, or you could use a private holder struct, all kinds of things).

The nice thing about this is you do get a reasonably consistent cross-platform interface. The compiler will tell you if you messed up a required function, you get one definition of default params, documentation, etc., you can even deliberately expose platform-specific things through that impl member, which indicates to users there's a different support expectation.

The bad thing is there's so much duplication and jumping around to figure out what does and doesn't actually work. Adding new features gets tedious as you do it over and over again.

The version spam copy/pasted in druntime is bad. And the static assert is too. But I'm not sure copying the file like this is the right answer. My gut feeling is having a central like core.config module that defines enums for the platform in a central location and then static if inside the function on that for the implementation would be an easier adaptation. idk if version list in said module or of the makefile generated it would be a better approach.

And that module can also be imported by other code to build on the same idea.

Did you consider this kind of static if thing instead?

@denizzzka
Copy link
Contributor Author

denizzzka commented Jan 12, 2024

Will such mixin approach cause problems with protected/package attributes?

but i don't really like this approach

Why?

  1. Ddoc problems
  2. Default arguments problems

More?

(and not just cuz it would break the build on my computer lol)

Hmmm....

import core.sync.platform.event_impl; // placeholder name

This means what we still need some non-cross-platform build code for switching modules

@adamdruppe
Copy link
Contributor

Will such mixin approach cause problems with protected/package attributes?

I don't think the mixin was actually a good solution; I regret using it and prefer to branch inside the function. I wouldn't separate the files at all.

This means what we still need some non-cross-platform build code for switching modules

This example was meant to show something I regret, not something I'm suggestion - my suggestion is to import the config module that has the tags then static if on them. But even if we did htis, there'd be no build code, you'd just

import core.config;
static if(pthreads_compatible)
   import core.sys.event.pthreads;
else static if(win32)
   import core.sys.event.win32;

no build system swap

@denizzzka
Copy link
Contributor Author

denizzzka commented Jan 12, 2024

import core.config;
static if(pthreads_compatible)
   import core.sys.event.pthreads;
else static if(win32)
   import core.sys.event.win32;

no build system swap

How to build druntime for platforms that not supported officially in this case?

@adamdruppe
Copy link
Contributor

by hitting make, the functions will just not be implemented then

@denizzzka
Copy link
Contributor Author

denizzzka commented Jan 12, 2024

by hitting make, the functions will just not be implemented then

Something like

externDFunc!("core.event.Event.setEvent",
                void function(void*) nothrow @nogc @safe);

externDFunc!("core.event.Event.clearEvent",
            void function(void*) nothrow @nogc @safe);

?

I tried it, it's disgusting

By the way, there is another advantage of my approach: if it doesn't work everything can be returned back by a script: all touched files located at same places of files hierarchy, so we will be able just merge it into version branches again

@adamdruppe
Copy link
Contributor

no, i'd just leave the implementation empty, or put a runtime assert(0) in that branch. Don't use static assert since that breaks the build, but a runtime assert can be tested incrementally.

@denizzzka
Copy link
Contributor Author

denizzzka commented Jan 12, 2024

no, i'd just leave the implementation empty

My goal is to make possible to implement any platform support without changing druntime sources. Without forks that adds support WebAsm or SerenityOS, so that a next fashioned breakthrough platform can be implemented in two weeks

@denizzzka
Copy link
Contributor Author

denizzzka commented Jan 12, 2024

I am not sure that documentation and default arguments is a big problem:

Current documentation also contains a lot of footnotes due to differences in platforms, no matter how much we want to maintain an abstracted interface. Therefore, it would be correct to generate documentation for different modules separately. And it can also be displayed beautifully in the web interface

Default arguments... It depends on what kind of argument it is. At one hand, baheviour of defaults may be covered by the tests. At other hand - that is why they are made by default.

Can we get automatically generated list of functions with default arguments? Maybe there are only a few of them and we shouldn’t worry about

@adamdruppe
Copy link
Contributor

My problem is that all this stuff is duplicated over several files so if you update one of the generic pieces it means several edits.

@denizzzka
Copy link
Contributor Author

denizzzka commented Jan 12, 2024

if you update one of the generic pieces it means several edits.

Let's hope on testing (and a spontaneous reduction of the number of such places during of code generalization)

For now files like event.d still require tests on all platforms, because they have several separate execution flows in parallel for a different platforms - not know which is worse

@denizzzka denizzzka marked this pull request as ready for review February 1, 2024 20:53
@denizzzka
Copy link
Contributor Author

My problem is that all this stuff is duplicated over several files so if you update one of the generic pieces it means several edits.

It is not advisable to avoid duplication everywhere. But for Event this is possible (see current version of this PR)

Also, as side effect, becomes obvious pointlessness of the handler checks before each Event method calling

@denizzzka
Copy link
Contributor Author

Is CI disabled?

@adamdruppe
Copy link
Contributor

it ran on my PR (even though it is a waste of time, i actually do want to disable it on PR things) but maybe not yours cuz you ... might have to rebase n master to get the ci script.

@denizzzka
Copy link
Contributor Author

denizzzka commented Feb 2, 2024

This is how ldc2.conf file looks:

[...]

"^wasm(32|64)-":
{
    switches = [
        "-defaultlib=",
        "-L-z", "-Lstack-size=1048576",
        "-L--stack-first",
        "-L--export-dynamic",
    ];
    lib-dirs = [];
};

".+-windows-.+":    { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/windows" ]; };
".+-osx-.+":        { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/osx" ]; };
".+-dragonfly-.+":  { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/dragonfly" ]; };
".+-freebsd-.+":    { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/freebsd" ]; };
".+-linux-.+":      { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/linux" ]; };
".+-netbsd-.+":     { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/netbsd" ]; };
".+-openbsd-.+":    { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/openbsd" ]; };
".+-sunos-.+":      { post-switches = [ "-I/tmp/include/d", "-I/tmp/include/d/platforms/sunos" ]; };

adamdruppe pushed a commit that referenced this pull request Feb 4, 2024
@adamdruppe
Copy link
Contributor

Gonna try the approach from the other PRs instead.

@adamdruppe adamdruppe closed this Feb 23, 2024
@denizzzka
Copy link
Contributor Author

Gonna try the approach from the other PRs instead.

For continuity: @adamdruppe probably means here PRs #45 #48 #49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants