Skip to content

Conversation

@heinezen
Copy link
Member

We use a .clang-format file for our code style but most code in the repo does not adhere to this style. This PR changes that.

Depends on #119

@heinezen heinezen added improvement improves existing functionality c++ involves C++ code labels Sep 15, 2024
@TheJJ
Copy link
Member

TheJJ commented Sep 16, 2024

pls rebase. and we should sync the openage clang-format config to the nyan repo while we're at it :)

@heinezen
Copy link
Member Author

pls rebase. and we should sync the openage clang-format config to the nyan repo while we're at it :)

done

Copy link
Member

@TheJJ TheJJ left a comment

Choose a reason for hiding this comment

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

hmm, before we push this, we should have another attempt to tame the formatting more. some formatting is now worse than before.

std::ostringstream builder;
builder << msg << ": "
<< token_type_str(token.get_type());
<< token_type_str(token.get_type());
Copy link
Member

Choose a reason for hiding this comment

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

hm this is incorrect now - i thought clang-format was able to mix indentation and alignment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that because << is technically a line continuation? The documentation says:

UT_AlignWithSpaces (in configuration: AlignWithSpaces) Use tabs for line continuation and indentation, and spaces for alignment.

Copy link
Member

Choose a reason for hiding this comment

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

it's a line continuation that should align to the previous <<. i thought we found a solution for this already to format correctly.
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignoperands

but it seems to be weird anyway, even with always-tab (which we don't have):
llvm/llvm-project#59797

Copy link
Member

@TheJJ TheJJ left a comment

Choose a reason for hiding this comment

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

the stream operation continuations are now non-aligned, but i can live with it.

the function chain calls are really messed up though. i've created llvm/llvm-project#114538 because this seems like a bug in clang-format.

we should manually fix them, it's not many places it seems.

@heinezen
Copy link
Member Author

heinezen commented Nov 2, 2024

Should I revert the changes that you mentioned for now? I don't think clang will solve them very soon.

We can wrap these code parts in clang-format: off, so they don't get touched in the future.

@TheJJ
Copy link
Member

TheJJ commented Nov 2, 2024

yea that may be a workaround until there's a fix

@heinezen
Copy link
Member Author

heinezen commented Nov 2, 2024

I did a few ther changes to avoid having to use clang-format: off.

@TheJJ TheJJ merged commit 3ebebd2 into SFTtech:master Nov 3, 2024
9 checks passed
@TheJJ
Copy link
Member

TheJJ commented Nov 3, 2024

great, good to go now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ involves C++ code improvement improves existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants