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

winapi: Add clang-format file #670

Merged
merged 1 commit into from
Jun 22, 2024
Merged

Conversation

Ryzee119
Copy link
Contributor

@Ryzee119 Ryzee119 commented Jun 15, 2024

Add a clang-format file specifcally to the winapi folder. The formatting across the lib/ varies alot with formatting so this atleast targets this one folder that has bitten me a few times with formatting issues., although this also works pretty well for the nxdk/ folder

This was based on the bare minimum custom options I could find whilst matching the existing format as best as I could.
This results in the following diff when ran against the winapi folder

2a2392a comments1
f22f6ee comments2
1a2ab65 comments3
Ryzee119@0b31421

The majority of these diffs look like genuine formatting issues.

Unfortuntely it does not handle our calling conventions __cdecl or __stdcall and removes a space when it shouldn't. I think it is related to this issue llvm/llvm-project#34171 and nothing I could do seemed to bypass it.

Where there was a mix of formatting I went with the majority (ie extern "c" { vs extern "c" \n {

Also dont know if we want a ColumnLimit ?

Copy link
Member

@thrimbor thrimbor left a comment

Choose a reason for hiding this comment

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

Looks mostly good.

The only thing I really don't like is how it reformats some if statements:
image

We've been avoiding the latter because it can lead to things like Apple's "goto fail" bug. Do you know whether clang-format has an option to disallow using if without braces entirely?

Also, what happened here? 😆
image

lib/winapi/.clang-format Outdated Show resolved Hide resolved
lib/winapi/.clang-format Outdated Show resolved Hide resolved
@Ryzee119
Copy link
Contributor Author

Do you know whether clang-format has an option to disallow using if without braces entirely?

There is this option?
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#insertbraces

@Ryzee119
Copy link
Contributor Author

Ryzee119 commented Jun 15, 2024

Pushed a new commit with fixes. New diff looks like

f22f6ee

Ill hightlight this oddity. Doesnt handle the short hand if/else. Could refactor or a clang-format-off/on wrap?

image

@JayFoxRox
Copy link
Member

I added some comments on 2a2392a and only realized afterwards that thrimbor made some of those arguments already (and that it wasn't the latest revision).

@Ryzee119 Ryzee119 force-pushed the winapi_format branch 2 times, most recently from 3e8a848 to 97f973d Compare June 16, 2024 04:39
@Ryzee119
Copy link
Contributor Author

I pushed another round. See top comment for new commit

Just need to agree on if we want to align #defines everywhere or not (winerror.h is not aligned, but minwinbase.h), or exclude this section only

Ill refactor this in a follow up before applying the formatting
image

@RadWolfie
Copy link
Contributor

Just need to agree on if we want to align #defines everywhere or not (winerror.h is not aligned, but minwinbase.h), or exclude this section only

In my honest opinion, anything that is grouped together for list of #defines should be align for easier reading.

@Ryzee119
Copy link
Contributor Author

Pushed new commit to make #defines line up if in same group. See top comment for latest diff

@JayFoxRox
Copy link
Member

LGTM.

Some minor notes:

  • I left a comment about GetDayOfWeek in Ryzee119@0b31421?diff=unified&w=0#r143421801.

  • I'm also not sure if windows.h or other kernel headers have special requirements for #include order (as different headers like to add macros which windows.h will respect, or windows.h needs to be included before those macros are added). So I assume we might have to split into different include blocks in the future. However, if the code still works as-is, then it's fine with me.

  • The alignment of #define blocks might need to a lot of reformatting diffs in the future (especially for #define Function FunctionA or constant tables where we manually add entries), so we should probably start https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view and ask users to reformat in a separate commit if necessary.

@thrimbor
Copy link
Member

Agreed, I'll merge now.
If someone creates a follow-up PR that does the reformatting, please make sure that commit id gets ignored in the blame view like jfr's link describes.
For GetDayOfWeek I'd be ok with temporarily excluding that part from reformatting until it's refactored.

@thrimbor thrimbor merged commit 2945074 into XboxDev:master Jun 22, 2024
6 checks passed
@thrimbor thrimbor mentioned this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants