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.

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

Jump to bottom

[GLUTEN-6900] Use clang-format-18 #6907

Closed
wants to merge 1 commit into from
Closed

[GLUTEN-6900] Use clang-format-18 #6907

wants to merge 1 commit into from
+30 −22

Conversation

xumingming
Copy link
Contributor

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

(Fixes: #6900)

@FelixYBW
Copy link
Contributor

Why do we need to upgrade clang-format?

@xumingming
Copy link
Contributor Author

xumingming commented Aug 20, 2024 •
edited
Loading

@FelixYBW The reason is as follows:

  1. When I change some c++ code, and submit the PR, the PR will fail at code style check.
  2. I noticed there is dev/formatcppcode.sh, but it only support Linux
  3. After make dev/formatcppcode.sh support macOS, I found that clang-format on macOS(which is of version 18) behaves differently from clang-format-15 on Linux.
  4. And there seems no easy way to install clang-format-15 on macOS.

Do you have suggestions?

FelixYBW
FelixYBW reviewed Aug 20, 2024
View reviewed changes
dev/formatcppcode.sh
echo "Installing $CLANG_FORMAT_CMD..."
sudo apt update
sudo apt install $CLANG_FORMAT_CMD
fi
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the install commands here. It's only for darwin and ubuntu.

@FelixYBW
Copy link
Contributor

@FelixYBW The reason is as follows:

  1. When I change some c++ code, and submit the PR, the PR will fail at code style check.
  2. I noticed there is dev/formatcppcode.sh, but it only support Linux
  3. After make dev/formatcppcode.sh support macOS, I found that clang-format on macOS(which is of version 18) behaves differently from clang-format-15 on Linux.
  4. And there seems no easy way to install clang-format-15 on macOS.

Do you have suggestions?

Different OS has different default clang-format version. e.g. I'm using Ubuntu20.04 which has clang-format-12. Do 12 and 18 have any difference on format? format-12 works well on my side so far.

@FelixYBW FelixYBW closed this Aug 20, 2024
@FelixYBW FelixYBW reopened this Aug 20, 2024
@xumingming
Copy link
Contributor Author

@FelixYBW The reason is as follows:

  1. When I change some c++ code, and submit the PR, the PR will fail at code style check.
  2. I noticed there is dev/formatcppcode.sh, but it only support Linux
  3. After make dev/formatcppcode.sh support macOS, I found that clang-format on macOS(which is of version 18) behaves differently from clang-format-15 on Linux.
  4. And there seems no easy way to install clang-format-15 on macOS.

Do you have suggestions?

Different OS has different default clang-format version. e.g. I'm using Ubuntu20.04 which has clang-format-12. Do 12 and 18 have any difference on format? format-12 works well on my side so far.

Yes, they have many difference. Seems not easy to get a same output on macOS, closing it.

@xumingming xumingming closed this Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FelixYBW FelixYBW FelixYBW left review comments

Assignees
No one assigned
Labels
BUILD INFRA VELOX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use newer version of clang-format(clang-format-18)
2 participants
@xumingming @FelixYBW