Apply clangformat to gpu code #4719

FabianSchuetze · 2021-04-22T07:10:21Z

As discussed with @kunaltyagi in PR #4677, we thought clang-formatting the GPU codebase in a batched commit might be helpful. This commit tries to do that. I applied clang-format with the following lines of code:

find . -name '*.h' -or -name '*.hpp' -or -name '*.cpp' | xargs /usr/bin/clang-format-10 -i -style=file

The command finds all files with h, hpp, or cpp ending recursively in the GPU folder and formats it according to the .clang-format file.

kunaltyagi · 2021-04-22T09:14:02Z

Would it be possible to split this PR? 50k lines of diff to parse is not easy

FabianSchuetze · 2021-04-22T12:16:11Z

Splitting the PR is a great idea! I will think of logical and manageable chunks and the commit again. Thanks for the comment!

FabianSchuetze · 2021-04-24T16:32:37Z

I reverted the previous commit and formatted only the code in the container folder. Once we have discussed these changes, I can commit another folder and we can discuss it again. We can keep the discussion confined to one PR and do not have to create several different PRs. Do you think this is a viable strategy, @kunaltyagi ?

kunaltyagi

Can we also add gpu/containers to the format.sh file?

gpu/containers/include/pcl/gpu/containers/device_array.h

kunaltyagi

4/10 files reviewed

PS: your format CI is failing

kunaltyagi · 2021-05-02T04:14:58Z

gpu/containers/include/pcl/gpu/containers/impl/device_array.hpp

-{ DeviceMemory::operator=(other); return *this; }
+template <class T>
+inline DeviceArray<T>::DeviceArray()
+{}


Let's separate these functions with newlines

Yes, that is an excellent idea! I wonder why clang-format cannot do that automatically for us, but I couldn't find a corresponding setting. You don't know one either?

I don't think there's an option for that: https://releases.llvm.org/11.0.0/tools/clang/docs/ClangFormatStyleOptions.html

gpu/containers/include/pcl/gpu/containers/impl/device_array.hpp

gpu/containers/include/pcl/gpu/containers/kernel_containers.h

kunaltyagi · 2021-05-02T04:18:55Z

You need to run clang-format 2-3 times till it settles on an equilibrium. See the functions offending the clang-format:
https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=18880&view=logs&j=84ba2080-ade3-55e3-f209-94ce73f9d406&t=9838d42f-a715-5615-242f-7e73bf03078b&l=1

FabianSchuetze · 2021-05-03T16:22:08Z

Thanks for your comments and explanations! I hope I did address your concerns; the files look much better now!

kunaltyagi

The print statements in initiaization.cpp don't look pretty, but apart from that, LGTM.

I'll approve if we can't improve the visual feel of print statements (it's a minor issue that's all)

kunaltyagi · 2021-05-05T07:35:50Z

gpu/containers/include/pcl/gpu/containers/device_memory.h

+  rows() const;
+
+  /** \brief Returns stride between two consecutive rows in bytes for internal buffer.
+   * Step is stored always and everywhere in bytes!!! */


I think the line regarding Step is stored... bytes!!! can be converted to a note or a warning. Not relevant to this PR

kunaltyagi · 2021-05-05T07:38:46Z

gpu/containers/include/pcl/gpu/containers/impl/device_array.hpp

-{ DeviceMemory::operator=(other); return *this; }
+template <class T>
+inline DeviceArray<T>::DeviceArray()
+{}


I don't think there's an option for that: https://releases.llvm.org/11.0.0/tools/clang/docs/ClangFormatStyleOptions.html

gpu/containers/src/initialization.cpp

FabianSchuetze · 2021-05-07T16:06:13Z

Thanks for the review, @kunaltyagi! The print statements in initialization.cpp are indeed not pretty. Once std::format is supported by the compilers, we could rewrite the print functions with output streams and std::format.

gpu/containers/include/pcl/gpu/containers/kernel_containers.h

This reverts commit 5171078.

kunaltyagi

minor comments aside, lgtm

gpu/containers/src/initialization.cpp

kunaltyagi · 2021-06-12T13:46:44Z

This has been a long time coming 😆

mvieth · 2021-06-13T19:43:52Z

@kunaltyagi Could you run the "Generate Documentation" CI to check if no doxygen warnings appear?

kunaltyagi · 2021-06-13T23:49:42Z

Done. https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=19298&view=results

FabianSchuetze · 2021-06-14T05:58:27Z

Thank you so much, Kunal and Markus, for guiding me through this!

mvieth

Seems fine

kunaltyagi reviewed Apr 27, 2021

View reviewed changes

gpu/containers/include/pcl/gpu/containers/device_array.h Outdated Show resolved Hide resolved

gpu/containers/include/pcl/gpu/containers/device_array.h Outdated Show resolved Hide resolved

kunaltyagi requested changes May 2, 2021

View reviewed changes

kunaltyagi reviewed May 5, 2021

View reviewed changes

FabianSchuetze mentioned this pull request May 7, 2021

Update support for CUDA arch in CMake and convertSMVer2Cores #4748

Merged

larshg reviewed May 17, 2021

View reviewed changes

gpu/containers/include/pcl/gpu/containers/kernel_containers.h Show resolved Hide resolved

FabianSchuetze added 11 commits May 21, 2021 12:29

applied clangformat to gpu code

74c66c9

Revert "applied clangformat to gpu code"

51bc57c

This reverts commit 5171078.

clangformat for containers

af01bc3

modified dev/format.sh

4ed1ff1

try to address doxgygen issues

9200333

fixed failing clang-format

7f5018e

newlines and line length

5a9ee8d

more newlines

c10cfa7

some small incongruencies

c2999ba

some incongruencies

0f548e3

ordering

eea673b

FabianSchuetze mentioned this pull request May 23, 2021

Allow gpu::Octree to work with arbitrary point types (tries to address #2218) #4764

Open

FabianSchuetze force-pushed the gpu_clangformat_batch branch from 15253cb to eea673b Compare May 26, 2021 16:04

kunaltyagi reviewed Jun 9, 2021

View reviewed changes

gpu/containers/src/initialization.cpp Show resolved Hide resolved

gpu/containers/src/initialization.cpp Outdated Show resolved Hide resolved

changed wording to fit on one line

ff3fea7

kunaltyagi approved these changes Jun 12, 2021

View reviewed changes

larshg approved these changes Jun 14, 2021

View reviewed changes

kunaltyagi requested a review from mvieth June 14, 2021 15:31

mvieth approved these changes Jun 14, 2021

View reviewed changes

kunaltyagi changed the title ~~applied clangformat to gpu code~~ Apply clangformat to gpu code Jun 15, 2021

kunaltyagi merged commit df4bd4a into PointCloudLibrary:master Jun 15, 2021

tin1254 pushed a commit to tin1254/pcl that referenced this pull request Aug 10, 2021

Apply clangformat to gpu code (PointCloudLibrary#4719)

cdde6f8

mvieth pushed a commit to mvieth/pcl that referenced this pull request Dec 27, 2021

Apply clangformat to gpu code (PointCloudLibrary#4719)

87d1f36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply clangformat to gpu code #4719

Apply clangformat to gpu code #4719

FabianSchuetze commented Apr 22, 2021

kunaltyagi commented Apr 22, 2021

FabianSchuetze commented Apr 22, 2021

FabianSchuetze commented Apr 24, 2021

kunaltyagi left a comment

kunaltyagi left a comment

kunaltyagi May 2, 2021

FabianSchuetze May 3, 2021

kunaltyagi May 5, 2021

kunaltyagi commented May 2, 2021

FabianSchuetze commented May 3, 2021

kunaltyagi left a comment

kunaltyagi May 5, 2021

kunaltyagi May 5, 2021

FabianSchuetze commented May 7, 2021

kunaltyagi left a comment

kunaltyagi commented Jun 12, 2021 •

edited

Loading

mvieth commented Jun 13, 2021

kunaltyagi commented Jun 13, 2021

FabianSchuetze commented Jun 14, 2021

mvieth left a comment

Apply clangformat to gpu code #4719

Apply clangformat to gpu code #4719

Conversation

FabianSchuetze commented Apr 22, 2021

kunaltyagi commented Apr 22, 2021

FabianSchuetze commented Apr 22, 2021

FabianSchuetze commented Apr 24, 2021

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi May 2, 2021

Choose a reason for hiding this comment

FabianSchuetze May 3, 2021

Choose a reason for hiding this comment

kunaltyagi May 5, 2021

Choose a reason for hiding this comment

kunaltyagi commented May 2, 2021

FabianSchuetze commented May 3, 2021

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi May 5, 2021

Choose a reason for hiding this comment

kunaltyagi May 5, 2021

Choose a reason for hiding this comment

FabianSchuetze commented May 7, 2021

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi commented Jun 12, 2021 • edited Loading

mvieth commented Jun 13, 2021

kunaltyagi commented Jun 13, 2021

FabianSchuetze commented Jun 14, 2021

mvieth left a comment

Choose a reason for hiding this comment

kunaltyagi commented Jun 12, 2021 •

edited

Loading