ladybird
ladybird copied to clipboard
AK: Simplify usage of windows.h and winsock2.h
Use <AK/windows.h> instead of windows.h/winsock2.h to avoid timeval-related errors.
Hello!
One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.
This is not like for like. Winsock isn't the same as windows. So if we name it windows.h we should be including windows.h. winsock is only for the sockets. Maybe we can include both in AK/Windows.h.
winsock2.h includes windows.h, no need to include both.
Didn't know that. Then it's fine. Is there anything else we might want here?
Maybe, we will know when we need that.
Any reason this starts with lower case specifically?
Just my preference.
This doesn't match convention in AK, nor the normal name for it. Causes a compilation error due to non-portable-include if you write with uppercase.
Not matching the AK convention is deliberate. I wanted this file to be different because it is a special platform header, it doesn't define any classes/functions in AK namespace. The same goes for sockaddr-win.h.
The "normal" name for <windows.h> debatable, but I think it is more lowercase than titlecase. MSDN examples use lowercase, Visual Studio templates use lowercase. I saw lowercase version much more often in real code. Also, this header is supposed to include everything from win32 API that is not included by other headers (like sockets and time), so it doesn't directly correspond to <windows.h> and doesn't have to follow its conventions.
But again, it is more a matter of preference. If maintainers make me change it I'll change it, but for now I want to keep lowercase.
But again, it is more a matter of preference. If maintainers make me change it I'll change it, but for now I want to keep lowercase.
Consider this a maintainer request to change the names of any headers added in your PRs to Titlecase, regardless of how platform-specific they are. :)
@ADKaster I changed it to title case, but note that AK already has similar platform-specific files, kmalloc.h/.cpp, and they use lowercase. I think you are wrong on this one.
@awesomekling Can you comment on this ^^^ ? (Platform-specific windows.h vs Windows.h in AK)
kmalloc.h is an ancient artifact from Serenity's kernel using AK. It is not a good argument to break naming conventions in new files.
@awesomekling Can you comment on this ^^^ ? (Platform-specific windows.h vs Windows.h in AK)
Please conform to project convention and name it Windows.h
@ADKaster I renamed the file and added AK/Platform.h, but was unable to get rid of #ifdef.
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.