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

ARROW-71: [C++] Add clang-tidy and clang-format to the the tool chain. #55

Closed
wants to merge 7 commits into from
Closed

ARROW-71: [C++] Add clang-tidy and clang-format to the the tool chain. #55

wants to merge 7 commits into from

Conversation

emkornfield
Copy link
Contributor

I changed the ubuntu flavor for building to precise because travis-ci/apt-source-safelist#199 is currently blocking using trusty.

I also expect there might be a couple of iterations on settings for clang-format and clang-tidy (or if we even want them as standard parts of the toolchain). @wesm I noticed the lint target explicitly turns off some checks, I don't know if these were copy and pasted or you really don't like them. If the latter I can do a first pass of turning the same ones off for clang-tidy.

In terms of reviewing: It is likely useful, to look at the PR commit by commit, since the last two commits are 99% driven by the first commit. The main chunk of code that wasn't machine fixed is FatalLog in logging.

The good news is clang-tidy caught one potential corner case segfault when a column happened to be null :)

emkornfield
emkornfield reviewed Apr 1, 2016
View reviewed changes
ci/travis_script_cpp.sh
@@ -7,6 +7,10 @@ set -e
pushd $CPP_BUILD_DIR

make lint
if [ $TRAVIS_OS_NAME == "linux" ]; then
make check-format
make clang-tidy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be check-clang-tidy

@emkornfield emkornfield changed the title [WIP] ARROW-71- Add clang-tidy and clang-format to the the tool chain. [WIP] ARROW-71- [C++] Add clang-tidy and clang-format to the the tool chain. Apr 1, 2016
@emkornfield emkornfield changed the title [WIP] ARROW-71- [C++] Add clang-tidy and clang-format to the the tool chain. ARROW-71: [C++] Add clang-tidy and clang-format to the the tool chain. Apr 1, 2016
wesm
wesm reviewed Apr 1, 2016
View reviewed changes
cpp/src/arrow/array-test.cc
void SetUp() {
pool_ = default_memory_pool();
}
void SetUp() { pool_ = default_memory_pool(); }
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, is this configurable? (I prefer the old style, but it's not a dealbreaker)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. The current configuration is to only do one liners for methods declared inside a class scope. The options are never have single line methods, only have single line methods for empty methods, class-scope or always make single line methods. We could likely work around this particular instance by adding a line comment to prevent the collapse.

My general preference is for never or class scope. There were a few instances where the class-scope inlining was already done, and looked right to me.

Copy link
Member

Choose a reason for hiding this comment

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

Let's stick with class scope, then, for now. Thanks

@emkornfield
adjust line length from 88 to 90, turn on bin packing of parameters. …
2fafb10 
…increase penality for before first call parameter
@wesm
Copy link
Member

wesm commented Apr 1, 2016

This seems good to me. If you're satisfied, will merge and we'll run with this?

@asfgit asfgit closed this in 5d12999 Apr 2, 2016
@emkornfield emkornfield deleted the emk_add_clang_tidy_PR branch April 9, 2016 05:32
@wesm
Copy link
Member

wesm commented May 1, 2016

@emkornfield according to Travis CI, they aren't in any hurry to whitelist the LLVM apt repos, but because we have sudo on the trusty platform, we can apt-add-repository and do whatever we want. So it should be possible to go back to Trusty and use LLVM 3.8 if we wish

wesm added a commit to wesm/arrow that referenced this pull request Sep 2, 2018
@wesm
PARQUET-468: Use thirdparty Thrift compiler to compile parquet.thrift…
71bf6e6 
… at make time

This also adds a `#define` working around the googletest incompatibility described in PARQUET-470.

Author: Wes McKinney <wesm@apache.org>

Closes apache#55 from wesm/PARQUET-468 and squashes the following commits:

e0338df [Wes McKinney] Auto-generate Thrift C++ bindings from environment Thrift compiler. Add #define workaround for Thrift >= 0.9.2 std::tuple conflict with googletest
wesm added a commit to wesm/arrow that referenced this pull request Sep 4, 2018
@wesm @julienledem
PARQUET-468: Use thirdparty Thrift compiler to compile parquet.thrift…
b59526d 
… at make time

This also adds a `#define` working around the googletest incompatibility described in PARQUET-470.

Author: Wes McKinney <wesm@apache.org>

Closes apache#55 from wesm/PARQUET-468 and squashes the following commits:

e0338df [Wes McKinney] Auto-generate Thrift C++ bindings from environment Thrift compiler. Add #define workaround for Thrift >= 0.9.2 std::tuple conflict with googletest

Change-Id: I2fce672b43352a15d3246b606a011b35efa54fe1
wesm added a commit to wesm/arrow that referenced this pull request Sep 6, 2018
@wesm @julienledem
PARQUET-468: Use thirdparty Thrift compiler to compile parquet.thrift…
d3fec7a 
… at make time

This also adds a `#define` working around the googletest incompatibility described in PARQUET-470.

Author: Wes McKinney <wesm@apache.org>

Closes apache#55 from wesm/PARQUET-468 and squashes the following commits:

e0338df [Wes McKinney] Auto-generate Thrift C++ bindings from environment Thrift compiler. Add #define workaround for Thrift >= 0.9.2 std::tuple conflict with googletest

Change-Id: I2fce672b43352a15d3246b606a011b35efa54fe1
wesm added a commit to wesm/arrow that referenced this pull request Sep 7, 2018
@wesm @julienledem
PARQUET-468: Use thirdparty Thrift compiler to compile parquet.thrift…
654cdda 
… at make time

This also adds a `#define` working around the googletest incompatibility described in PARQUET-470.

Author: Wes McKinney <wesm@apache.org>

Closes apache#55 from wesm/PARQUET-468 and squashes the following commits:

e0338df [Wes McKinney] Auto-generate Thrift C++ bindings from environment Thrift compiler. Add #define workaround for Thrift >= 0.9.2 std::tuple conflict with googletest

Change-Id: I2fce672b43352a15d3246b606a011b35efa54fe1
wesm added a commit to wesm/arrow that referenced this pull request Sep 8, 2018
@wesm @julienledem
PARQUET-468: Use thirdparty Thrift compiler to compile parquet.thrift…
218fe8e 
… at make time

This also adds a `#define` working around the googletest incompatibility described in PARQUET-470.

Author: Wes McKinney <wesm@apache.org>

Closes apache#55 from wesm/PARQUET-468 and squashes the following commits:

e0338df [Wes McKinney] Auto-generate Thrift C++ bindings from environment Thrift compiler. Add #define workaround for Thrift >= 0.9.2 std::tuple conflict with googletest

Change-Id: I2fce672b43352a15d3246b606a011b35efa54fe1
xuechendi pushed a commit to xuechendi/arrow that referenced this pull request Aug 4, 2020
@zhouyuan
Merge pull request apache#55 from zhouyuan/wip_hdfs_kerberos
0fd3c62 
[C++]Wip hdfs kerberos
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.
2 participants
@emkornfield @wesm