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

Eliminate shared this from std/socket.d #5472

Closed

Conversation

andralex
Copy link
Member

This ends up simplifying Posix a lot. It could be simplified even further.

@andralex andralex changed the title Nomoresharedzisinsocketlastoneyay Eliminate shared this from std/socket.d Jun 11, 2017
@andralex
Copy link
Member Author

@CyberShadow
Copy link
Member

I think we can now get rid of getnameinfo being conditional by now. It was starting to lose sense when it was introduced in 2011, and today we probably don't support anything that old because of other reasons.

@CyberShadow
Copy link
Member

BTW, I'm skeptical about replacing static ~this with atexit.

  • Is atexit thread-safe? man says it's thread-safe but what about TLS variables? Will the function specified to atexit be called in the same thread as the code calling atexit?
  • Do the semantics match up correctly? Such as thread/process distinctions, and unloading of DLLs?
  • atexit seems to have a limit of 32 functions (ATEXIT_MAX).
  • Not sure about the merits of replacing a D feature with a C feature.

@andralex
Copy link
Member Author

I think we can now get rid of getnameinfo being conditional by now.

OK, I'll look into that.

I think we're good with atexit only as long as we only mess with other C libraries.

@andralex
Copy link
Member Author

@CyberShadow just to make sure: on Posix getnameinfo, getaddrinfo, and freeaddrinfo can never be null weak symbols. Correct?

@CyberShadow
Copy link
Member

Needs updates to documentation (e.g. lots of references to SocketFeatureException don't apply any more).

@andralex
Copy link
Member Author

@CyberShadow done.

All - I notice that setKeepAlive throws depending on a statically known condition, which seems kinda silly. Should we just static assert(false) in that case? Then we can remove SocketFeatureException altogether.

@CyberShadow
Copy link
Member

Nice work. Would be good to see the project tester's results for this one (so no auto-merge so far).

All - I notice that setKeepAlive throws depending on a statically known condition, which seems kinda silly.

Yeah, that's a bit of a Java-ism.

Should we just static assert(false) in that case?

It would mean making the function a template. Currently it's virtual due to virtual-by-default, and because of that Socket methods are all pretty sensitive to breakage (see issue 16514).

I guess there's also the small risk that there is code out there that already has setKeepAlive behind a regular if (not static).

@andralex
Copy link
Member Author

Cool, so I guess this PR is good to go. @CyberShadow ?

std/socket.d Outdated
}

// Now that we called WSAStartup, make sure we clean up, too.
import core.stdc.stdlib;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import core.stdc.stdlib : atexit; as per CircleCI

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh. Not a fan of this rule. I recall we introduced it to compensate for hijacking, which we since fixed.

@CyberShadow
Copy link
Member

Still lots of errors in the autotester.

@andralex andralex force-pushed the nomoresharedzisinsocketlastoneyay branch from 50d1048 to 07bd22c Compare June 30, 2017 17:43
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@andralex
Copy link
Member Author

OK, pushed fixes to the autotester problems. Flying blindly here...

@andralex andralex force-pushed the nomoresharedzisinsocketlastoneyay branch from 07bd22c to 73b0f6e Compare June 30, 2017 18:01
@andralex
Copy link
Member Author

moar fixes

@andralex andralex force-pushed the nomoresharedzisinsocketlastoneyay branch from 73b0f6e to 82b92dd Compare July 4, 2017 17:02
@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

Hmmm, so now we have a different problem:

Error 42: Symbol Undefined _getnameinfo@28

It seems this is caused by switching from dynamic loading to statically linking. What library do we need to link with, and what's the cmdline syntax? Thx!

@CyberShadow
Copy link
Member

You'll need to update the import libraries... :/

There are some definition files in druntime/def, but none for winsock. The best way may be to have the DMC import libraries updated. @WalterBright?

@WalterBright
Copy link
Member

What library do we need to link with, and what's the cmdline syntax?

Use grep, the all-purpose tool:

grep getnameinfo \dm\lib\*.lib

Unfortunately, no hits. Trying:

grep getnameinfo \dm\bin\*.dll

No hits there, either. I don't know where it is supposed to be; it isn't in the DMC distribution.

@CyberShadow
Copy link
Member

I don't know where it is supposed to be; it isn't in the DMC distribution.

You need to add getnameinfo and getaddrinfo (possibly others?) to ws2_32.lib.

@WalterBright
Copy link
Member

You need to add getnameinfo and getaddrinfo (possibly others?) to ws2_32.lib.

The dm\bin\coffimplib.exe program can be used to convert 32 bit MS-Coff files to OMF.

@andralex andralex force-pushed the nomoresharedzisinsocketlastoneyay branch from 82b92dd to 485f015 Compare October 25, 2017 21:31
@andralex
Copy link
Member Author

Reverted the pull that causes problems on Windows.

@andralex
Copy link
Member Author

This should be good to go subject to the autotester passing.

@andralex
Copy link
Member Author

I'm throwing away this entire pile of dung and start anew.

@andralex
Copy link
Member Author

Redone in #5813

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.

6 participants