I asked about this on github but didn't get a good answer, so I'm asking here.  What's with all the repeated OS blocks in druntime?

https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/unistd.d#L945
https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/netinet/in_.d#L974
https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/arpa/inet.d#L201

It seems like pointless repetition and there are many more examples of this, as I keep running into it when adding bionic/Android support.  Martin suggested that it would be useful to have a default case that asserts for an unsupported OS, but many of these blocks don't have that either.

Why not just declare them once for Posix and then specialize for a particular OS later on when necessary, as seems to have been done in these instances?

https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/sys/stat.d#L954
https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/unistd.d#L95
https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/signal.d#L425

Note that the last version(Posix) check in core.sys.posix.signal on line 479 is always going to be hit.  You could argue that it's worth it just to document that that is the last case we expect to hit, but then the static assert after that is pointless.

As can be seen from these links, druntime is not consistent in how it's separating declarations for different OSs, sometimes using the latter, more compact declarations, other times highly redundant for no apparent reason.  If we can hash out how it should be done, I'll submit a pull to fix it up.

On 12/12/2014 04:47 PM, Joakim wrote:
> I asked about this on github but didn't get a good answer, so I'm asking
> here.  What's with all the repeated OS blocks in druntime?

No, you don't want to accept the answer. That's slightly different than not getting none.

> https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/unistd.d#L945
>
> https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/netinet/in_.d#L974
>
> https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/arpa/inet.d#L201
>
>
> It seems like pointless repetition and there are many more examples of
> this, as I keep running into it when adding bionic/Android support.
> Martin suggested that it would be useful to have a default case that
> asserts for an unsupported OS, but many of these blocks don't have that
> either.

Because it was written before we spread out to multiple architectures, and learned hard it is to add something.

>
> Why not just declare them once for Posix and then specialize for a
> particular OS later on when necessary, as seems to have been done in
> these instances?

We've been through this several times, because the poor guy adding support for a new OS or arch has the find all the differences through debugging.

> As can be seen from these links, druntime is not consistent in how it's

Have you ever seen software with more than a 100 LOC that's totally consistent? There have been many contributors, the project is several years old...

> If we can hash out how it should be done, I'll submit a pull to
> fix it up.

There nothing unclear here.

version (A)
else version (B)
else static assert(0, "unimplemented");

As Daniel pointed out, if you run into something that's extremely want to save some you might factor out common stuff into a default block.

version (A)
    import ...default;
else version (B)
    import ...default;
else version (C)
    something else
else
    static assert(0, "unimplemented");

That's already problematic for a reviewer, because it's much harder to check the correctness of a patch (when comparing it with the C headers).

On 12/12/2014 06:13 PM, Martin Nowak wrote:
> On 12/12/2014 04:47 PM, Joakim wrote:
>> I asked about this on github but didn't get a good answer, so I'm asking
>> here.  What's with all the repeated OS blocks in druntime?
>
> No, you don't want to accept the answer. That's slightly different than
> not getting none.

s/none/one/ :)

On 12/12/2014 06:13 PM, Martin Nowak wrote:
> No, you don't want to accept the answer. That's slightly different than
> not getting none.

Let me amend that I'm glad you didn't run away and are still working on separating kernel specific from libc specific stuff. That will help us to mitigate the combinatorial explosion of Kernel x Arch x Libc.

On 12 Dec 2014 17:15, "Martin Nowak via Digitalmars-d" < digitalmars-d@puremagic.com> wrote:
>
> On 12/12/2014 04:47 PM, Joakim wrote:
>>
>> I asked about this on github but didn't get a good answer, so I'm asking here.  What's with all the repeated OS blocks in druntime?
>
>
> No, you don't want to accept the answer. That's slightly different than
not getting none.
>
>
>>
https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/unistd.d#L945
>>
>>
https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/netinet/in_.d#L974
>>
>>
https://github.com/D-Programming-Language/druntime/blob/master/src/core/sys/posix/arpa/inet.d#L201
>>
>>
>> It seems like pointless repetition and there are many more examples of this, as I keep running into it when adding bionic/Android support. Martin suggested that it would be useful to have a default case that asserts for an unsupported OS, but many of these blocks don't have that either.
>
>
> Because it was written before we spread out to multiple architectures,
and learned hard it is to add something.
>
>
>>
>> Why not just declare them once for Posix and then specialize for a particular OS later on when necessary, as seems to have been done in these instances?
>
>
> We've been through this several times, because the poor guy adding
support for a new OS or arch has the find all the differences through debugging.
>
>

One of the worst I've hit was a crash deep in the start-up initialisation of Druntime... caused by a totally not obvious struct size/align mismatch with Cruntime's pthread.  Along with not wasting a day switching between druntime and system C headers, having a compile-time error ensures that the porter has vetted the correctness of the ported declarations and structures.

It may take a while to port all areas, but at least you can be mostly assured that each step is towards a fully working runtime.

Iain.

On Friday, 12 December 2014 at 15:47:09 UTC, Joakim wrote:
>
> It seems like pointless repetition and there are many more examples of this, as I keep running into it when adding bionic/Android support.  Martin suggested that it would be useful to have a default case that asserts for an unsupported OS, but many of these blocks don't have that either.
>
> Why not just declare them once for Posix and then specialize for a particular OS later on when necessary

Because therein lies madness.


> as seems to have been done in these instances?

I could be convinced to allow required function prototypes to be
in version(Posix) blocks, but never data definitions.  And even
putting function prototypes in common version blocks risks link
errors.  I would *love* it if platforms that contained a header
actually implemented all of the functions required by a specific
tag (required, xopen, etc) when choosing any one from that tag,
but even this can't be relied upon.

In my C/C++ code I have compatibility modules that conditionally
implement missing functions based on the platform and libc
version, but this is so not fun.  Particularly when you're
targeting as many platform as D is trying to.  So really, you
should never see a version(Posix) block in core.sys.posix,
because it means that for some platform, compilation/linking will
fail.


> As can be seen from these links, druntime is not consistent in how it's separating declarations for different OSs, sometimes using the latter, more compact declarations, other times highly redundant for no apparent reason.  If we can hash out how it should be done, I'll submit a pull to fix it up.

It should all be redundant.  It makes for large files, but is
far easier to maintain and debug against than mixing everything
together.

On 12 Dec 2014 21:50, "Sean Kelly via Digitalmars-d" < digitalmars-d@puremagic.com> wrote:
>
> On Friday, 12 December 2014 at 15:47:09 UTC, Joakim wrote:
>>
>>
>> It seems like pointless repetition and there are many more examples of
this, as I keep running into it when adding bionic/Android support.  Martin suggested that it would be useful to have a default case that asserts for an unsupported OS, but many of these blocks don't have that either.
>>
>> Why not just declare them once for Posix and then specialize for a
particular OS later on when necessary
>
>
> Because therein lies madness.
>
>
>
>> as seems to have been done in these instances?
>
>
> I could be convinced to allow required function prototypes to be in version(Posix) blocks, but never data definitions.  And even putting function prototypes in common version blocks risks link errors.  I would *love* it if platforms that contained a header actually implemented all of the functions required by a specific tag (required, xopen, etc) when choosing any one from that tag, but even this can't be relied upon.
>
> In my C/C++ code I have compatibility modules that conditionally implement missing functions based on the platform and libc version, but this is so not fun. Particularly when you're targeting as many platform as D is trying to.  So really, you should never see a version(Posix) block in core.sys.posix, because it means that for some platform, compilation/linking will fail.
>

Except maybe at the top with extern(Posix):

The compiler (or build environment) can at least work out that it is targeting a vaguely POSIX platform.

On Friday, 12 December 2014 at 17:14:01 UTC, Martin Nowak wrote:
> On 12/12/2014 04:47 PM, Joakim wrote:
>> I asked about this on github but didn't get a good answer, so I'm asking
>> here.  What's with all the repeated OS blocks in druntime?
>
> No, you don't want to accept the answer. That's slightly different than not getting none.

As I alluded to before, saying that the repeated OS blocks are needed for default cases, which should assert if an OS is not included in the list, and then admitting that none of them have such default cases is not a good answer.

> Because it was written before we spread out to multiple architectures, and learned hard it is to add something.

None of the linked examples have to do with multiple architectures either, so this is not going to help you with that.

> We've been through this several times, because the poor guy adding support for a new OS or arch has the find all the differences through debugging.

The linked examples do not change having to debug in the slightest.  They merely add more work to find all these version(OS) blocks, for no apparent reason, at least if the porter wants to be complete with all the files.  Or the porter can just ignore them, since they don't assert anyway.

>> As can be seen from these links, druntime is not consistent in how it's
>
> Have you ever seen software with more than a 100 LOC that's totally consistent? There have been many contributors, the project is several years old...

I'm well aware of the reasons, but that's no reason not to make it consistent now.

> > If we can hash out how it should be done, I'll submit a pull
> to
>> fix it up.
>
> There nothing unclear here.
>
> version (A)
> else version (B)
> else static assert(0, "unimplemented");

It is currently unclear when this is actually necessary, as I've pointed out with my examples.  Any Posix OS is free to differ on any of hundreds of C declarations.  Trying to guess ahead of time where that hypothetical OS might vary seems like premature optimization to me.  In each of the examples I gave, none of the major Posix OSs differ for those declarations, yet we're separating them for no reason.  Finally, since you admit that druntime is inconsistent, there must be something "unclear" or druntime wouldn't be inconsistent.

> As Daniel pointed out, if you run into something that's extremely want to save some you might factor out common stuff into a default block.
>
> version (A)
>     import ...default;
> else version (B)
>     import ...default;
> else version (C)
>     something else
> else
>     static assert(0, "unimplemented");
>
> That's already problematic for a reviewer, because it's much harder to check the correctness of a patch (when comparing it with the C headers).

Yeah, not going to do that either.  My point is that there are several cases where all this versioning is being done pointlessly.  Doing it once up at the top of the module does not fix the issue.

On Friday, 12 December 2014 at 18:49:08 UTC, Iain Buclaw via Digitalmars-d wrote:
> One of the worst I've hit was a crash deep in the start-up initialisation
> of Druntime... caused by a totally not obvious struct size/align mismatch
> with Cruntime's pthread.  Along with not wasting a day switching between
> druntime and system C headers, having a compile-time error ensures that the
> porter has vetted the correctness of the ported declarations and structures.
>
> It may take a while to port all areas, but at least you can be mostly
> assured that each step is towards a fully working runtime.

Since none of the examples given would produce compile-time errors, your claims are wrong.

On Friday, 12 December 2014 at 21:47:37 UTC, Sean Kelly wrote:
> On Friday, 12 December 2014 at 15:47:09 UTC, Joakim wrote:
>> Why not just declare them once for Posix and then specialize for a particular OS later on when necessary
>
> Because therein lies madness.

Yet, that is the way it's currently done for the latter three examples I gave from druntime.

>> as seems to have been done in these instances?
>
> I could be convinced to allow required function prototypes to be
> in version(Posix) blocks, but never data definitions.  And even
> putting function prototypes in common version blocks risks link
> errors.  I would *love* it if platforms that contained a header
> actually implemented all of the functions required by a specific
> tag (required, xopen, etc) when choosing any one from that tag,
> but even this can't be relied upon.

Yes, I'm aware of this, as I've seen such unimplemented function declarations in bionic.  But the vast majority of C function declarations in druntime are already in common version(Posix) blocks, the one at the top of the module.  I'm merely suggesting we do the same for the few holdouts that are being separated for no apparent reason.  As for data definitions, I don't see why they're special, but I do notice now that more enums are separated by OS in druntime.

> In my C/C++ code I have compatibility modules that conditionally
> implement missing functions based on the platform and libc
> version, but this is so not fun.  Particularly when you're
> targeting as many platform as D is trying to.  So really, you
> should never see a version(Posix) block in core.sys.posix,
> because it means that for some platform, compilation/linking will
> fail.

I believe "else version(Posix) {decl;}" is currently used synonymously for "else {decl;}", merely to underline at that point in the source that the default case is Posix, even though it's redundant from the "version (Posix):" at the top of the module.  I understand that you probably don't want either, whereas I'm suggesting replacing the first three examples with just a single declaration that's not inside any version block, except of course the one at the top of the module.

>> As can be seen from these links, druntime is not consistent in how it's separating declarations for different OSs, sometimes using the latter, more compact declarations, other times highly redundant for no apparent reason.  If we can hash out how it should be done, I'll submit a pull to fix it up.
>
> It should all be redundant.  It makes for large files, but is
> far easier to maintain and debug against than mixing everything
> together.

When should it be redundant, for every single Posix declaration in druntime?  That's far from the case now.  When the authors think a handful of declarations might vary on some future unspecified platform?  Each of the linked examples are separated, yet none of them vary on any of the 4-5 supported Posix platforms.  In fact, I just checked and none of them are used anywhere in druntime and phobos, with only IPPROTO_RAW used to define another unused enum in phobos.

My guess is that you put each of these inside a version(linux) block just to be safe when you first started druntime, and porters have since been blindly adding other OSs to the list, despite the separation serving no real purpose.  I have no problem with having version(OS) blocks where there are actual differences between the OSs, just not these cases where all the OSs happen to be the same.

I'm suggesting cleaning up this needless redundancy, to make it easier for future porters.

On Saturday, 13 December 2014 at 15:58:29 UTC, Joakim wrote:
> When should it be redundant, for every single Posix declaration in druntime?  That's far from the case now.  When the authors think a handful of declarations might vary on some future unspecified platform?  Each of the linked examples are separated, yet none of them vary on any of the 4-5 supported Posix platforms.  In fact, I just checked and none of them are used anywhere in druntime and phobos, with only IPPROTO_RAW used to define another unused enum in phobos.
>
> My guess is that you put each of these inside a version(linux) block just to be safe when you first started druntime, and porters have since been blindly adding other OSs to the list, despite the separation serving no real purpose.  I have no problem with having version(OS) blocks where there are actual differences between the OSs, just not these cases where all the OSs happen to be the same.
>
> I'm suggesting cleaning up this needless redundancy, to make it easier for future porters.

Since I didn't get a response to this, I've submitted a work-in-progress pull with the suggested cleanups, along with others talked about in this thread:

https://github.com/D-Programming-Language/druntime/pull/1069

Any further discussion can take place on the PR thread.