search

ptitSeb / box64

Box64 - Linux Userspace x86_64 Emulator with a twist, targeted at ARM64 Linux devices
https://box86.org
MIT License
3.73k stars 267 forks source link

refactor: format code #1387

Closed f1yby closed 6 months ago

tristanmorgan commented 6 months ago

Possibly adding a lint step to the GH-Actions could be used to enforce formatting for the future.

ksco commented 6 months ago

I suppose you’re using the .clang-format file I added a while ago, here are the concerns about using it globally:

  1. A massive reformat like this one will cause big trouble on git blame, which happens to be a feature I personally used a lot.
  2. The .clang-format file can do some bad formatting on some wild macros (look at what it did to the IFX macro for example).
  3. There is no point in doing so unless we enforce it in the CI, but I believe @ptitSeb thinks that’s “boring” according to what we discussed before.

Anyway, currently the .clang-format file is there for the usage of formatting newly added code by some of the contributors like me as they find it convenient for things like aligning backslashes.

f1yby commented 6 months ago

I suppose you’re using the .clang-format file I added a while ago, here are the concerns about using it globally:

  1. A massive reformat like this one will cause big trouble on git blame, which happens to be a feature I personally used a lot.
  2. The .clang-format file can do some bad formatting on some wild macros (look at what it did to the IFX macro for example).
  3. There is no point in doing so unless we enforce it in the CI, but I believe @ptitSeb thinks that’s “boring” according to what we discussed before.

Anyway, currently the .clang-format file is there for the usage of formatting newly added code by some of the contributors like me as they find it convenient for things like aligning backslashes.

  1. GIt Blame is sacrificed, maybe you can blame first to the format commit, and blame to the commit which introduce the problem.
  2. clang format can be turned off for some limes using // clang-format off and turned on using // clang-format on, or just ignore all macros, or we can tag IFX as ifmacros
  3. just for readability, but compared to other black magic used in box64, formation is really not a big problem. Yes, it's boring to some extent, because it's a drop in a bucket for readability

EDIT: Could you further describe what looking bad happened to IFX? I think it's ok

ksco commented 6 months ago

I'm expecting IFX(X_ALL) { instead of IFX(X_ALL)\n{ since we treat it visually as an if, but I think the ifmacros you mentioned should be enough addressing this.

f1yby commented 6 months ago

I think a compromise solution is to format files changed in the future and use the github action to force it. But before that, we should agree on which is better for reading, formatting or not.

ptitSeb commented 6 months ago

I really really really don't want my commit to be red because I missed a space between if and (. That upset me a lot, so my project will not enforce that.

And I don't like that all files are changed is such drastic way.

I don't think I'll merge this one. But fill free to create a PR with the modified clang-format, so new files / new parts can be formated if contributor want to use that.

ksco commented 6 months ago

Yeah, the clang-format file can have a update indeed, especially the IFX rule.

f1yby commented 6 months ago

I update the .clang-format in this pr. Now it is free for everyone who wants to format the code to do the stuff like find -iname '*.cpp' -o -iname '*.c' -o -iname '*.h' | xargs clang-format -i if they prefer viewing the code with format. The step by step format maybe a better solution for the project, and the format rule is ready for it.

ptitSeb commented 6 months ago

Note that there is no c++ file in box64. The cpp files present in the repo are from a side project aim to automatize library wrapping (and I don't mind if those file gets refortated).

I have merged the update of the format rules and will close this PR.