Skip to content

Conversation

@ericcurtin
Copy link
Collaborator

@ericcurtin ericcurtin commented Nov 14, 2024

So people can use things like clang-format and git clang-format to
style their code. Copied from ggml-cann, with PointerAlignment
changed to Middle.

@ericcurtin ericcurtin force-pushed the clang-format branch 2 times, most recently from 5a68676 to f36ecea Compare November 14, 2024 12:19
@slaren
Copy link
Member

slaren commented Nov 14, 2024

I believe that having a .clang-format would be beneficial even if it is not applied by default on every source file, but at least we should make an effort to replicate the current style used in the core of the library. A big one would be PointerAlignment, it should be set to Middle.

@ericcurtin
Copy link
Collaborator Author

I believe that having a .clang-format would be beneficial even if it is not applied by default on every source file, but at least we should make an effort to replicate the current style used in the core of the library. A big one would be PointerAlignment, it should be set to Middle.

I agree. I find it particularly handy I can bounce around multiple codebases throughout any given year and it's very hard to remember the style nuances and quickly write code.

git clang-format is very good at just styling sections, so we don't have to apply globally.

@slaren if I copy paste this one:

ggml/src/ggml-cann/.clang-format

and change PointerAlignment to middle we should be good?

@slaren
Copy link
Member

slaren commented Nov 14, 2024

@slaren if I copy paste this one:

ggml/src/ggml-cann/.clang-format

and change PointerAlignment to middle we should be good?

I don't know, but I wouldn't expect so. The CANN backend has its own style that is not likely to be based on the core ggml/llama.cpp style. We tend to use lots of vertical alignment as well.

@ggerganov
Copy link
Member

For me the vertical alignment is really important. If we find a way to make it compatible with .clang-format, or at least not affect it, we can consider it.

@slaren
Copy link
Member

slaren commented Nov 14, 2024

I don't expect that we will be able to replicate exactly the same style (we don't even have a well-defined style in the first place), but it has a quite a few options for alignment. https://clang.llvm.org/docs/ClangFormatStyleOptions.html

@ericcurtin
Copy link
Collaborator Author

Not opinionated on what it looks like but I do want a .clang-format file so I can worry about manually modifying style less. Would it be easier for you guys to open a PR or will I keep iterating?

@ggerganov
Copy link
Member

I don't expect that we will be able to replicate exactly the same style (we don't even have a well-defined style in the first place), but it has a quite a few options for alignment. clang.llvm.org/docs/ClangFormatStyleOptions.html

Yes, these look like what we need.

@ericcurtin Start by copying the .clang-format from the CANN backend in the root and we'll iterate for some time to make it match the existing styles as much as we can. For now, don't auto-apply it on the full source files - just on the changed lines.

@ericcurtin ericcurtin closed this Nov 15, 2024
@ericcurtin ericcurtin deleted the clang-format branch November 15, 2024 12:09
@ericcurtin ericcurtin restored the clang-format branch November 15, 2024 12:09
@ericcurtin ericcurtin deleted the clang-format branch November 15, 2024 12:09
@ericcurtin ericcurtin restored the clang-format branch November 15, 2024 12:10
@ericcurtin ericcurtin deleted the clang-format branch November 15, 2024 12:11
@ericcurtin
Copy link
Collaborator Author

Sorry I closed this PR by mistake and it became unreopenable, new one here:

#10308

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants