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

[ci] Shift to clang-format-10 to resolve bug in clang-format-{7-9} #3895

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

kunaltyagi
Copy link
Member

clang-format-8 has a bug which causes some incorrect formatting. Shifting to v10 would resolve that.

Also modified is the cpp standard in the .clang-format. Should that be Cpp14 instead of Latest?

@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet module: ci changelog: fix Meta-information for changelog generation needs: feedback Specify why not closed/merged yet labels Apr 8, 2020
@kunaltyagi kunaltyagi self-assigned this Apr 8, 2020
@kunaltyagi
Copy link
Member Author

@diivm
Copy link
Contributor

diivm commented Apr 8, 2020

Any reason why we are shifting to v10 as opposed to v11?

@kunaltyagi
Copy link
Member Author

Latest version available by default in Ubuntu 20.04 LTS. The diff is similar and migration to 11 later (when 20.04 or 20.10 or smthg adds it) will be easy.

@taketwo
Copy link
Member

taketwo commented Apr 9, 2020

I'm going to push a new image to DockerHub and then trigger re-run here.

@taketwo taketwo merged commit 0e9f0f3 into PointCloudLibrary:master Apr 9, 2020
@SunBlack
Copy link
Contributor

SunBlack commented Apr 9, 2020

@divmadan v11 is not published yet, so it would be not the best idea to switch to an unstable version ;-)

Another question: Why Standard: Latest and not Standard: c++14, as we don't support C++17 or later?

@taketwo
Copy link
Member

taketwo commented Apr 9, 2020

Another question: Why Standard: Latest and not Standard: c++14, as we don't support C++17 or later?

Fair point.

Sorry for hastily merging this without enough time for a proper discussion. But I needed to have this in master to unblock other PRs since the fmt Docker image was already updated and formatting check started to fail without this PR.

@diivm
Copy link
Contributor

diivm commented Apr 9, 2020

Just to add a point, Latest and Cpp11 gave similar diffs for our whitelisted directories.

@kunaltyagi kunaltyagi deleted the clang-format branch April 10, 2020 00:53
@kunaltyagi
Copy link
Member Author

Should we have a PR for Cpp14 as the standard? Or is sticking to "Latest" for formatting purpose ok?

@taketwo
Copy link
Member

taketwo commented Apr 10, 2020

Probably? Our code is C++14 code, not C++20, so why confusing the formatter?

@taketwo taketwo changed the title [ci] Shift to clang-format-10 to resolve bug in clang-format-{7-9} [ci] Shift to clang-format-10 to resolve bug in clang-format-{7-9} May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Meta-information for changelog generation module: ci needs: code review Specify why not closed/merged yet needs: feedback Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants