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 compatibility note for clang-format and fix a small issue in .clang-format #3877

Closed

Conversation

SunBlack
Copy link
Contributor

@SunBlack SunBlack commented Apr 7, 2020

I tried to make clang-format 10 compatible to 7-9, but I didn't found a way to do so. So basically I compared result of clang-format-8 -dump-config > .clang-format8 with clang-format-10 -dump-config > .clang-format10. As no default value of an option was changed and we cannot use newer options, as defining them breaks clang-tidy for old version (you can't tell clang-tidy to ignore unknown options) and even no new option sounds like it is the reason for the slightly other formatting, I added a note that PCL is only compatible to clang-tidy 7-9 and not 10.

Example of difference between clang-tidy 8 and 10:
image

@taketwo
Copy link
Member

taketwo commented Apr 7, 2020

👍 for the fix in .clang-format

Regarding the formatting change, it seems to me that the old clang-format has a bug that was fixed in the newer version. This is the current formatting:

/** \brief Get reference to container */
ContainerT& operator*() { return container_; }
/** \brief Get const reference to container */
const ContainerT&
getContainer() const
{
return container_;
}

Apparently, when formatting operator members, old clang-format does not respect our rules about one-liners and template declaration breaking.

I think the way forward is to switch to the newest version (with correct behavior) on CI and add a note that old versions make mistakes.

@SunBlack
Copy link
Contributor Author

SunBlack commented Apr 7, 2020

Switching to clang-format 10 also also an option, as the result looks better for me

Addition: The difference between clang-format 8 and 10 basing on your .clang-format file:
image

@kunaltyagi
Copy link
Member

I'd vote for switching to 10, and reporting that in certain conditions, the output may differ (which has always been the case)

@SunBlack
Copy link
Contributor Author

Closing in favor of #3903.

@SunBlack SunBlack closed this Apr 10, 2020
@SunBlack SunBlack deleted the clang-format_compatibility branch April 10, 2020 19:27
truhoang pushed a commit to truhoang/pcl that referenced this pull request Jun 30, 2020
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