search

odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.39k stars 562 forks source link

Fix signatures and add some in Windows #3671

Closed reduf closed 2 months ago

reduf commented 2 months ago

Fix few issues I notice while working with Odin and added the definition of two new functions on Windows.

Kelimion commented 2 months ago

Please don't make pull requests for untested changes. This breaks winsock bindings.

reduf commented 2 months ago

Do you mind re-opening the pr? I was obviously going to address the compilation failure in the examples.

Kelimion commented 2 months ago

I do, yes.

GetQueuedCompletionStatusEx doesn't take a sockaddr, nor do pton and ntop, but with no explanation at all* you've changed and broken 5 APIs unrelated to your patch.

* "Fix few issues I notice while working with Odin and added the definition of two new functions on Windows."

So no, I'm not going to reopen this pull request, because if we don't merge the commit that breaks things, we don't need to then have another commit that fixes it adding needless churn (and making git bisect users grumpy in the process).

reduf commented 2 months ago

GetQueuedCompletionStatusEx doesn't take a sockaddr

nor does this pr change that, it changes ^OVERLAPPED_ENTRY to [^]OVERLAPPED_ENTRY which seems correct considering that GetQueuedCompletionStatusEx takes an array of OVERLAPPED_ENTRY.

I'm not sure either why you mention pton or ntop. I mean, your statement that they don't take sockaddr isn't technically incorrect, it just doesn't make sense in this context.

All the other API changed here were correctly fixed, look at the documentation, the link are literally above every APIs. I'm not sure what you want me to explain. I just copied what was in the doc. If you don't see the same info in the doc, would you mind sharing which doc you're using?

Kelimion commented 2 months ago

I'm not sure either why you mention pton or ntop. I mean, your statement that they don't take sockaddr isn't technically incorrect, it just doesn't make sense in this context.

https://github.com/odin-lang/Odin/pull/3671/files

I'm bringing them up because you added them. That's fine if that's all that happened, but their presence doesn't explain why you broke the other APIs that use winsock. I bring it up because you broke 5 API's in the same family without justification, and I point out that adding pton and ntop clearly can't have been your justification for doing so.

"Fix few issues I notice while working with Odin." also doesn't explain why you broke accept, getsockname, getpeername, bind, and connect.

All the other API changed here were correctly fixed, look at the documentation, the link are literally above every APIs. I'm not sure what you want me to explain.

Something that used to work and no longer did after you changed the signature is fixed?! image

Get out of here.