Skip to content

[style] Introduce clang-format #6895

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[style] Introduce clang-format #6895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
25f7e0d
Fix
borchero Apr 27, 2025
6f532ec
Make fixes
borchero Apr 27, 2025
1f0014a
Fix config
borchero Apr 27, 2025
72397f0
Fix
borchero Apr 27, 2025
e160e7f
Ignore
borchero Apr 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .ci/lint-cpp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ set -e -E -u -o pipefail

echo "running cpplint"
cpplint \
--filter=-build/c++11,-build/include_subdir,-build/header_guard,-whitespace/line_length \
--filter=-build/c++11,-build/include_subdir,-build/header_guard,-whitespace/line_length,-readability/braces \
--recursive ./src ./include ./R-package ./swig ./tests \
|| exit 1
echo "done running cpplint"
Expand Down
4 changes: 4 additions & 0 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
BasedOnStyle: Google
Copy link
Collaborator

@jameslamb jameslamb Apr 27, 2025

Choose a reason for hiding this comment

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

Suggested change
BasedOnStyle: Google
# configuration for 'clang-format'
#
# for details, see https://clang.llvm.org/docs/ClangFormatStyleOptions.html
BasedOnStyle: Google

Can you please add a comment here on how to find these configuration options' meanings and the list of possible options?

ColumnLimit: 99
# Sorting includes currently breaks the code so we have to disable it
Copy link
Collaborator

@jameslamb jameslamb Apr 27, 2025

Choose a reason for hiding this comment

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

Can you elaborate on this, please? What specifically does "breaks the code" mean?

If there are issues caused by the ordering of includes, it would be good to identify them... for example, is LightGBM #define-ing things with names that conflict with #defines from third-party headers?

It'd be fine to document that in a separate issue and link the issue in the comment in this file, but I'd like to see a bit more documentation here than just "breaks the code", please.

Copy link
Collaborator Author

@borchero borchero Apr 27, 2025

Choose a reason for hiding this comment

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

If there are issues caused by the ordering of includes, it would be good to identify them...

I agree. I can create an issue but I'm not sure how to comprehensively identify files where the import order plays a role as compilation fails on the first file where there is an issue 👀

Copy link
Collaborator

@jameslamb jameslamb Apr 28, 2025

Choose a reason for hiding this comment

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

Thank you! Totally fine, no need to fully investigate the problem just to open the issue.

At least having the plaintext error message(s) from the compiler would be helpful for anyone else facing a similar thing to find it from search, and for anyone wanting to contribute to know where to start.

SortIncludes: Never
1 change: 1 addition & 0 deletions .clang-format-ignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
src/io/config_auto.cpp
Copy link
Collaborator

@jameslamb jameslamb Apr 27, 2025

Choose a reason for hiding this comment

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

I see this in the PR description:

Ignore most parts of include/LightGBM/config.h and all of src/io/config_auto.cpp from formatting (the latter via .clang-format-ignore) as clang-format messes up the current parameter documentation otherwise

How specifically does clang-format "mess it up"? Is this your personal opinion about the style produced by clang-format, or do you mean that it actually breaks the ability to generate https://lightgbm.readthedocs.io/en/latest/Parameters.html?

We always have the option to modify https://github.com/microsoft/LightGBM/blob/master/.ci/parameter-generator.py to generate differently-formatted code to work with these tools.

Copy link
Collaborator Author

@borchero borchero Apr 27, 2025

Choose a reason for hiding this comment

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

include/LightGBM/config.h

This file is ignored in parts (via // clang-format off and // clang-format on comments) as comments are re-formatted, breaking the current logic that extracts the documentation. I don't think it's worth making a lot of changes there now, i.e. I think disabling formatting there is reasonable.

src/io/config_auto.cpp

This file would currently be needed to be reformatted by clang-format. I agree that we can adjust the code generation though so that we don't need to ignore it from formatting.

Copy link
Collaborator

@jameslamb jameslamb Apr 28, 2025

Choose a reason for hiding this comment

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

Ok, thanks for the explanation! Totally fine with that. I do wish that this didn't involve adding 2 new root-level config files, but I understand that just excluding files in .pre-commit-config.yaml probably would mean that IDEs would still make clang-format suggestions about src/io/config_auto.cpp.

I'm ok with leaving this as-is.

5 changes: 5 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ repos:
hooks:
- id: yamllint
args: ["--strict"]
- repo: https://github.com/pre-commit/mirrors-clang-format
Copy link
Collaborator

@jameslamb jameslamb Apr 27, 2025

Choose a reason for hiding this comment

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

Some questions about how this will affect the development process.

  • does this require installing clang separately, or will pre-commit set that up for you?
  • does this work on Windows?

rev: v20.1.0
hooks:
- id: clang-format
types_or: [c++, c, cuda]
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.9.10
Expand Down
Loading
Loading