Skip to content

ARROW-1219: [C++] Use Google C++ code formatting#848

Closed
wesm wants to merge 2 commits intoapache:masterfrom
wesm:google-style
Closed

ARROW-1219: [C++] Use Google C++ code formatting#848
wesm wants to merge 2 commits intoapache:masterfrom
wesm:google-style

Conversation

@wesm
Copy link
Member

@wesm wesm commented Jul 15, 2017 •
edited
Loading

Our coding style guide has a ton of rules. I put up this patch to discuss since we don't have too many patches outstanding right now. This uses the exact Google style used in TensorFlow and other projects, but relaxes the column limit to 90 characters.

The main change is horizontal alignment in function signatures:

 void ValidateBasicStructArray(const StructArray* result,
-    const vector<uint8_t>& struct_is_valid, const vector<char>& list_values,
-    const vector<uint8_t>& list_is_valid, const vector<int>& list_lengths,
-    const vector<int>& list_offsets, const vector<int32_t>& int_values) {
+                              const vector<uint8_t>& struct_is_valid,
+                              const vector<char>& list_values,
+                              const vector<uint8_t>& list_is_valid,
+                              const vector<int>& list_lengths,
+                              const vector<int>& list_offsets,
+                              const vector<int32_t>& int_values) {

I find the paren-aligned version a bit more readable, but it's a matter of taste

@wesm
Copy link
Member Author

wesm commented Jul 15, 2017

@pcmoritz @robertnishihara @cpcloud @xhochy do you have any feelings on this? I think the Google style is more similar to what the Plasma codebase was prior to importing into Arrow.

@wesm wesm force-pushed the google-style branch 2 times, most recently from 14456d7 to 613f87e Compare July 15, 2017 21:34
@wesm wesm changed the title WIP ARROW-1219: [C++] Use Google C++ code formatting ARROW-1219: [C++] Use Google C++ code formatting Jul 15, 2017
@robertnishihara
Copy link
Contributor

robertnishihara commented Jul 15, 2017

I like the change aesthetically, the simpler clang-format, and the consistency with other projects. However, I don't feel strongly about this.

@pcmoritz
Copy link
Contributor

pcmoritz commented Jul 16, 2017

+1 for the change

@xhochy
Copy link
Member

xhochy commented Jul 16, 2017

I'm open for everything as long as ninja format works. Another thing that bothers me more: Could we pin the clang-format version? I have different versions on different systems and everywhere I would be able to install 3.8 , 3.9 and 4.0 but am unsure which one we currently use and thus often have reformatting changes in unrelated code.

Also I would merge the other open C++ PRs first before doing any reformatting to avoid huge merge conflicts, esp #797.

@wesm
Copy link
Member Author

wesm commented Jul 16, 2017

ninja format and make format work definitely. According to http://apt.llvm.org/ LLVM 3.9 is the "stable" version, but maybe this is a typo. I've been using that. I am happy to pin 4.0 as long as that is easy to obtain on both Linux and macOS. I just opened https://issues.apache.org/jira/browse/ARROW-1221 about this

It's not necessary to merge this right away. I can wait until #797 and other PRs are merged and then do the reformat

@wesm wesm closed this Jul 24, 2017
@wesm
Use Google C++ code formatting
8690257 
Change-Id: I277b40f5e68aa0b2fabb9e553a3a5ca96b4a43d7
@wesm wesm reopened this Jul 24, 2017
@wesm
Move import to platform.h
9e8fe4f 
Change-Id: I01fe37e0109433e156315977000da6181fad199f
@wesm
Copy link
Member Author

wesm commented Jul 25, 2017

Bombs away

@asfgit asfgit closed this in 07b89bf Jul 25, 2017
@wesm wesm deleted the google-style branch July 25, 2017 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wesm @robertnishihara @pcmoritz @xhochy