unnecessary OS redundancy in druntime

[Joakim via Digitalmars-d]

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.