ladybird icon indicating copy to clipboard operation
ladybird copied to clipboard

AK: Simplify usage of windows.h and winsock2.h

Open stasoid opened this issue 1 year ago • 15 comments

Use <AK/windows.h> instead of windows.h/winsock2.h to avoid timeval-related errors.

stasoid avatar Dec 01 '24 09:12 stasoid

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.

ladybird-bot avatar Dec 01 '24 09:12 ladybird-bot

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.

R-Goc avatar Dec 01 '24 11:12 R-Goc

winsock2.h includes windows.h, no need to include both.

stasoid avatar Dec 01 '24 11:12 stasoid

Didn't know that. Then it's fine. Is there anything else we might want here?

R-Goc avatar Dec 01 '24 11:12 R-Goc

Maybe, we will know when we need that.

stasoid avatar Dec 01 '24 11:12 stasoid

Any reason this starts with lower case specifically?

R-Goc avatar Dec 04 '24 13:12 R-Goc

Just my preference.

stasoid avatar Dec 04 '24 13:12 stasoid

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.

R-Goc avatar Dec 04 '24 13:12 R-Goc

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.

stasoid avatar Dec 04 '24 14:12 stasoid

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 avatar Dec 09 '24 00:12 ADKaster

@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.

stasoid avatar Dec 09 '24 02:12 stasoid

@awesomekling Can you comment on this ^^^ ? (Platform-specific windows.h vs Windows.h in AK)

stasoid avatar Dec 09 '24 02:12 stasoid

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.

trflynn89 avatar Dec 09 '24 03:12 trflynn89

@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

awesomekling avatar Dec 09 '24 08:12 awesomekling

@ADKaster I renamed the file and added AK/Platform.h, but was unable to get rid of #ifdef.

stasoid avatar Dec 09 '24 13:12 stasoid

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.

github-actions[bot] avatar Dec 18 '24 04:12 github-actions[bot]