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

Add .clang-format file #10292

Closed
wants to merge 0 commits into from
Closed

Conversation

ericcurtin
Copy link
Contributor

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

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
Contributor 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
Collaborator

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
Owner

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
Collaborator

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
Contributor 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
Owner

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
Contributor 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