www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - unnecessary OS redundancy in druntime

reply "Joakim" <dlang joakim.fea.st> writes:
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.
Dec 12 2014
next sibling parent reply Martin Nowak <code+news.digitalmars dawg.eu> writes:
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).
Dec 12 2014
next sibling parent Martin Nowak <code+news.digitalmars dawg.eu> writes:
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/ :)
Dec 12 2014
prev sibling next sibling parent Martin Nowak <code+news.digitalmars dawg.eu> writes:
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.
Dec 12 2014
prev sibling next sibling parent Iain Buclaw via Digitalmars-d <digitalmars-d puremagic.com> writes:
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.



 Dec 12 2014



prev sibling  parent reply "Joakim" <dlang joakim.fea.st>  writes:


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.



 Dec 13 2014



  parent  "Joakim" <dlang joakim.fea.st>  writes:


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.



 Dec 16 2014



prev sibling  parent reply "Sean Kelly" <sean invisibleduck.org>  writes:


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.



 Dec 12 2014



  parent  Iain Buclaw via Digitalmars-d <digitalmars-d puremagic.com>  writes:


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.



 Dec 12 2014