Skip to content

Conversation

@SYaoJun
Copy link

@SYaoJun SYaoJun commented Jan 31, 2026

use clang-tidy fix the warning

@SYaoJun SYaoJun closed this Jan 31, 2026
@SYaoJun SYaoJun reopened this Jan 31, 2026
@SYaoJun
Copy link
Author

SYaoJun commented Jan 31, 2026

#480

@SYaoJun
Copy link
Author

SYaoJun commented Feb 3, 2026

Hi @leerho, could you please review this PR? I'd appreciate your feedback.

@AlexanderSaydakov
Copy link
Contributor

I am not sure I support changing short one-line if statements like this
from

if (condition) return 0;

to

if (condition) {
return 0;
}

We did not establish any style requirements, and we use one-liners throughout the C++ library. So why should we change the approach in this particular class?

There is no harm, of course, but this style is a bit more verbose. I would not mind switching to this style if the community wants.

@SYaoJun
Copy link
Author

SYaoJun commented Feb 5, 2026

I am not sure I support changing short one-line if statements like this from

if (condition) return 0;

to

if (condition) { return 0; }

We did not establish any style requirements, and we use one-liners throughout the C++ library. So why should we change the approach in this particular class?

There is no harm, of course, but this style is a bit more verbose. I would not mind switching to this style if the community wants.

I appreciate your perspective. While this individual style change might seem minor, I believe we should think bigger: implementing a .clang-tidy configuration would provide long-term benefits for code consistency and maintainability.

I'd like to propose we adopt a .clang-tidy from another Apache project as a reference, then refactor this code to comply with it. As the codebase grows, having explicit linting rules becomes essential. Without them, we risk inconsistency and harder maintenance down the road.

Does this sound reasonable to you? We could start by researching configurations from similar Apache projects.

@leerho
Copy link
Member

leerho commented Feb 6, 2026

I tend to favor this idea, because I really like clean, readable, and robust code.

In the ds-Java library we use Checkstyle when possible and a checkstyle.xml config file is checked in. (E.g., Checkstyle is not compatible with Java 25 yet)

However, I would only agree to this if

  1. We agree as a group on the configuration rules. (E.g., “if (condition) { return 0; }” has been proven to be more robust against accidental edits. ) This should be done as the first step.
  2. someone volunteers to go through the existing C++ code and fix and carefully review the current style discrepancies. An automatic fixer tool could be a disaster! That is a big job. And neither Alex nor I have the time to do it. We could really use some help 🥹.

It would be ideal if we had something similar for all the languages 🙂.

@SYaoJun
Copy link
Author

SYaoJun commented Feb 6, 2026

I tend to favor this idea, because I really like clean, readable, and robust code.

In the ds-Java library we use Checkstyle when possible and a checkstyle.xml config file is checked in. (E.g., Checkstyle is not compatible with Java 25 yet)

However, I would only agree to this if

  1. We agree as a group on the configuration rules. (E.g., “if (condition) { return 0; }” has been proven to be more robust against accidental edits. ) This should be done as the first step.
  2. someone volunteers to go through the existing C++ code and fix and carefully review the current style discrepancies. An automatic fixer tool could be a disaster! That is a big job. And neither Alex nor I have the time to do it. We could really use some help 🥹.

It would be ideal if we had something similar for all the languages 🙂.

Thanks for the explanation — I’m aligned with this approach.

I’m willing to volunteer to go through the existing C++ code, carefully review the style discrepancies, and apply fixes without relying on an automatic formatter.

Once the team agrees on the Checkstyle configuration, I can start working on this.

For reference, here is the Apache Arrow .clang-tidy configuration:
https://github.com/apache/arrow/blob/main/.clang-tidy

If the team agrees on adopting this configuration (or a close variant of it), I can address the formatting issues incrementally, algorithm by algorithm.

@AlexanderSaydakov
Copy link
Contributor

While I personally prefer brief one-line style, I don't mind adopting a stricter approach with mandatory curly brackets if we consistently use it throughout the whole code in C++.

@leerho
Copy link
Member

leerho commented Feb 9, 2026 via email

@coveralls
Copy link

Pull Request Test Coverage Report for Build 21539762366

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.794%

Totals Coverage Status
Change from base Build 21387083530: 0.0%
Covered Lines: 17046
Relevant Lines: 17254

💛 - Coveralls

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.

4 participants