Clean up unnecessary version(Posix) and version(OS) and try to more narrowly scope catch-all else blocks

[joakim-noah]

As mentioned on the newsgroup, this pull starts cleaning up unnecessarily separated version(OS) blocks, so that porters aren't given unneeded work. Also, inside modules that are only available to Posix, version(Posix) checks are removed and catch-all uses of else version(Posix) are expanded to name each OS instead, as Sean said is the way it should be done.

I'm putting this pull up before it's done so that I can get comment on any of the proposed cleanups early on, before I spend time tracking more of these down.

[joakim-noah]

@rainers, while working on this pull, I just noticed that core.stdc.fenv still uses the old version (DigitalMars) version (Win32) formulation by default, ie it doesn't differentiate for 32-bit MSVCRT, mistake?

[joakim-noah]

Updated this pull, think I'm done with the first three cleanups. I'm now looking for catch-all else{ dec; } blocks after version statements and more narrowly defining them if possible, for example, changing version(Posix) { dec;} else { dec2;} blocks to version(Posix) { dec;} else version(Windows) { dec2;} in core.thread. I'm going to modify Dscanner to automate looking for more of these.

Also, I noticed that core.sync.barrier and core.sync.rwmutex were importing core.sys.windows.windows only for Win32, but that didn't seem to affect their tests on Win64, so I'm assuming those imports are unneeded and getting rid of them. Let's see if that passes the auto-tester.

[rainers]

@rainers, while working on this pull, I just noticed that core.stdc.fenv still uses the old version (DigitalMars) version (Win32) formulation by default, ie it doesn't differentiate for 32-bit MSVCRT, mistake?

Yes, it seems an oversight. Grepping for similar code, there are also core.sys.windows.stacktrace and std.process which should use the new versions.

[joakim-noah]

Updated this pull with the last cleanups, should be done and ready for review.

I modified Dscanner to find all version blocks with catch-all else blocks. I tried to filter out those that were merely there for a static assert, but that wasn't working so I left them in. Here's the patch I used:

diff --git a/src/symbol_finder.d b/src/symbol_finder.d
index 32ef86b..8e4d30b 100644
--- a/src/symbol_finder.d
+++ b/src/symbol_finder.d
@@ -99,7 +99,25 @@ class FinderVisitor : ASTVisitor
                }
        }

-       override void visit(const FunctionBody) {}
+       override void visit(const FunctionBody fb) {fb.accept(this);}
+
+       override void visit(const ConditionalDeclaration cd)
+        {
+            if (cd.compileCondition.versionCondition && cd.falseDeclaration &&
+                !cd.falseDeclaration.conditionalDeclaration)
+                output.writefln("got into an else with no conditional for version in %s at line %d",
+                              fileName, cd.compileCondition.versionCondition.token.line);
+           cd.accept(this);
+        }
+
+       override void visit(const ConditionalStatement cs)
+        {
+            if (cs.compileCondition.versionCondition && cs.falseStatement &&
+                ((cs.falseStatement.statement && !cs.falseStatement.statement.statementNoCaseNoDefault.conditionalStatement) || (cs.falseStatement.declaration && !cs.falseStatement.declaration.conditionalDeclaration)))
+                output.writefln("gots into an else with no conditional for version in %s at line %d",
+                              fileName, cs.compileCondition.versionCondition.token.line);
+           cs.accept(this);
+        }

        mixin template generateVisit(T)
        {

I ran the patched Dscanner on druntime:

find src -name "*.d" | xargs ../Dscanner/bin/dscanner -d no_decls > else.list

It found 273 instances. I then checked each of them manually and modified those that could be more narrowly scoped.

In two instances in core.sys.posix, I actually went the other way of the cleanups above and replaced an else with an else version(Posix), as that will make them easier to find and possibly expand later.

[joakim-noah]

@redstar, let me know if you need a Solaris declaration here, as there wasn't one in the phobos module this was moved from, but that predated your recent Solaris work.

[andralex]

Don't you need to place else branches with assert(false) at the end of OS choice static if/else statements?

[joakim-noah]

Don't you need to place else branches with assert(false) at the end of OS choice static if/else statements?

Currently, this is inconsistently done in druntime. Where I expanded out those cases to add more OS's, I left the last case the way it was. That's not really a focus of this patch: is there any particular instance from this PR you had in mind?

One cleanup that is included here is collapsing such version(OS) blocks where all the OS's happen to be defining the exact same declarations, ie there's no difference between all of them, into a single declaration. However, that is somehow controversial among some reviewers, though they haven't been able to give a single reason why.

[joakim-noah]

Updated this pull to use the newer CRuntime_Glibc/CRuntime_Bionic as done in #1010, rather than the older linux/Android. I also replaced the default else version(Posix) block from core.sys.posix.pthread with a static assert, rather than deleting it completely.

[andralex]

When we port to a new platform, is it possible that INET6_ADDRSTRLEN is something else than 46? A quick google search shows that it might be 48: http://lxr.free-electrons.com/source/include/linux/inet.h#L52. On other platforms it's indeed 46: http://www.castaglia.org/proftpd/doc/devel-guide/src/utils/utils.h.html.

So the first question is whether 46 is the right number on all platforms .The second is whether just defining it 46 for all platforms improves the state of affairs. With the previous code we'd have an undefined identifier error. With the new code we'd just have a constant that may be wrong.

I stopped here. For all cases there must be a clear case the code will fail (either at compile time or link time) if the platform has not been ported properly yet. Silent defaults are dangerous.

[joakim-noah]

From doing the same google search, I see webpages that state that it's defined to be 46 in both the Posix and IPv6 standards. I cannot tell what that platform is where it's 48, can you? If not, there's no point in dealing with it.

I think avoiding unnecessarily redundant definitions definitely "improves the state of affairs," particularly for future porters to new platforms. The question of whether a set of repeated definitions is unnecessary will have to be determined on a case-by-case basis.

The previous code would not give you an undefined identifier error if it was 48, as that header you linked appears to be for linux, which is one of the cases in the list. So it would have silently failed previously, as it would now. Nothing has changed with this pull for the case you bring up.

Speculatively separating declarations with no reason given is premature optimization, and we all know what that's the root of, right? ;)

[andralex]

I am definitely not convinced.

[joakim-noah]

I just debunked the one example you looked at, I don't know what else to tell you.

[MartinNowak]

If you made a technical argument, that an IP6 address can't be longer than 46 characters or had shown the declaration in glibc (https://sourceware.org/git/?p=glibc.git;a=blob;f=inet/netinet/in.h;hb=98734cc50153c80047f4ed9c6772bc7e1e68c9f7#l233) which actually implements inet_ntop, then you could have convinced someone that the Linux declaration is wrong.

[joakim-noah]

Saying that "it's defined to be 46 in both the Posix and IPv6 standards," as I did above, is a much stronger technical argument than pointing out the glibc declaration, as you just did. I can't help it if my technical arguments are being ignored.

[joakim-noah]

Updated this pull to compile for Android/x86 with the latest changes in HEAD, specifically modifying core.sys.posix.sys.resource to remove a default else block and adding the newly needed constant _SC_THREAD_STACK_MIN to core.sys.posix.unistd.

[joakim-noah]

Ping, can I get a review on this pull? It's been sitting largely unreviewed for a month and a half now. Most of it should be uncontroversial, simply cleanups that are already done elsewhere or have been asked for.

The only part that isn't is that I collapsed redundantly defined declarations into a single declaration in 14 locations. If that isn't wanted, somebody needs to articulate the reasons and what the current alternative is. So far, all I've gotten is a lot of hand-waving claims that the status quo is better, with no reasons given, and when a single example was looked at above, the claims didn't stand up to scrutiny.

[mathstuf]

Another ping.

[MartinNowak]

What's the reason that IPPROTO_RAW has to be identical in every kernel?

[joakim-noah]

No reason that I know of. Is there any reason why signal has to be provided by the local libc on every platform? Because it isn't in the libc in bionic, it's in the headers (lines 117-126), which means the linked druntime definition doesn't work and will need an alias to bsd_signal for bionic. I just noticed this yesterday, when trying to link a small app using signal, which is why the alias isn't there yet.

This could be true for practically every single declaration in druntime, ie they may be different than we expect. Are you suggesting that we put every single declaration in druntime in such a version(OS) block, just in case we need to change it later? That would make druntime 10 times bigger.

My point has always been that if IPPROTO_RAW is the same on all currently supported platforms, clearly there's no need to break it out like this right now. If we eventually support a platform where it's different, let's break out the version(OS) block then. If we already know it's different for a platform that we'd like to support, fine, leave this version block in along with a comment why you're doing it. But nobody has been able to provide such concrete reasons for all these blocks. If they did, I'd gladly put the comment in and leave that version block alone.

[joakim-noah]

Since collapsing unnecessarily separated version(OS) blocks proved controversial, I've spun that off into a separate PR #1216, so that particular cleanup doesn't affect the merging of these remaining needed cleanups.

[joakim-noah]

Ping, review needed, should be pretty straightforward.

[MartinNowak]

Sorry for the long delay.