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

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 :)

Author: Micah Kornfield <emkornfield@gmail.com>

Closes #55 from emkornfield/emk_add_clang_tidy_PR and squashes the following commits:

2fafb10 [Micah Kornfield] adjust line length from 88 to 90, turn on bin packing of parameters. increase penality for before first call parameter
169352f [Micah Kornfield] add llvm tool chain as travis source
e7723d1 [Micah Kornfield] upgrade to precise to verify if build works.  address self comments
d3f76d8 [Micah Kornfield] clang format change
9c556ef [Micah Kornfield] cleanup from clang-tidy
26945e9 [Micah Kornfield] add more failure checks for build_thirdparty
4dd0b81 [Micah Kornfield] Add clang-format and clang-tidy targets to toolchain

.travis.yml

@@ -1,11 +1,14 @@
 sudo: required
-dist: trusty
+dist: precise 
 addons:
  apt:
  sources:
  - ubuntu-toolchain-r-test
  - kalakris-cmake
+ - llvm-toolchain-precise-3.7
  packages:
+ - clang-format-3.7
+ - clang-tidy-3.7
  - gcc-4.9 # Needed for C++11
  - g++-4.9 # Needed for C++11
  - gdb

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 check-clang-tidy
+fi
 
 ctest -L unittest
 

cpp/CMakeLists.txt

@@ -30,10 +30,11 @@ set(THIRDPARTY_DIR "${CMAKE_SOURCE_DIR}/thirdparty")
 # Must be declared in the top-level CMakeLists.txt.
 set(CMAKE_SKIP_INSTALL_ALL_DEPENDENCY true)
 
-# Generate a Clang compile_commands.json "compilation database" file for use
-# with various development tools, such as Vim's YouCompleteMe plugin.
-# See http://clang.llvm.org/docs/JSONCompilationDatabase.html
-if ("$ENV{CMAKE_EXPORT_COMPILE_COMMANDS}" STREQUAL "1")
+find_package(ClangTools)
+if ("$ENV{CMAKE_EXPORT_COMPILE_COMMANDS}" STREQUAL "1" OR CLANG_TIDY_FOUND)
+ # Generate a Clang compile_commands.json "compilation database" file for use
+ # with various development tools, such as Vim's YouCompleteMe plugin.
+ # See http://clang.llvm.org/docs/JSONCompilationDatabase.html
  set(CMAKE_EXPORT_COMPILE_COMMANDS 1)
 endif()
 
@@ -540,6 +541,36 @@ if (UNIX)
  `find ${CMAKE_CURRENT_SOURCE_DIR}/src -name \\*.cc -or -name \\*.h | sed -e '/_generated/g'`)
 endif (UNIX)
 
+
+############################################################
+# "make format" and "make check-format" targets
+############################################################
+if (${CLANG_FORMAT_FOUND})
+ # runs clang format and updates files in place.
+ add_custom_target(format ${BUILD_SUPPORT_DIR}/run-clang-format.sh ${CMAKE_CURRENT_SOURCE_DIR} ${CLANG_FORMAT_BIN} 1
+ `find ${CMAKE_CURRENT_SOURCE_DIR}/src -name \\*.cc -or -name \\*.h | sed -e '/_generated/g'`)
+
+ # runs clang format and exits with a non-zero exit code if any files need to be reformatted
+ add_custom_target(check-format ${BUILD_SUPPORT_DIR}/run-clang-format.sh ${CMAKE_CURRENT_SOURCE_DIR} ${CLANG_FORMAT_BIN} 0
+ `find ${CMAKE_CURRENT_SOURCE_DIR}/src -name \\*.cc -or -name \\*.h | sed -e '/_generated/g'`)
+endif()
+
+
+############################################################
+# "make clang-tidy" and "make check-clang-tidy" targets
+############################################################
+if (${CLANG_TIDY_FOUND})
+ # runs clang-tidy and attempts to fix any warning automatically
+ add_custom_target(clang-tidy ${BUILD_SUPPORT_DIR}/run-clang-tidy.sh ${CLANG_TIDY_BIN} ${CMAKE_BINARY_DIR}/compile_commands.json 1
+ `find ${CMAKE_CURRENT_SOURCE_DIR}/src -name \\*.cc | sed -e '/_generated/g'`)
+ # runs clang-tidy and exits with a non-zero exit code if any errors are found.
+ add_custom_target(check-clang-tidy ${BUILD_SUPPORT_DIR}/run-clang-tidy.sh ${CLANG_TIDY_BIN} ${CMAKE_BINARY_DIR}/compile_commands.json 
+ 0 `find ${CMAKE_CURRENT_SOURCE_DIR}/src -name \\*.cc | sed -e '/_generated/g'`)
+
+endif()
+
+
+
 ############################################################
 # Subdirectories
 ############################################################

cpp/README.md

@@ -61,3 +61,19 @@ variables
 * Googletest: `GTEST_HOME` (only required to build the unit tests)
 * Google Benchmark: `GBENCHMARK_HOME` (only required if building benchmarks)
 * Flatbuffers: `FLATBUFFERS_HOME` (only required for the IPC extensions)
+
+## Continuous Integration
+
+Pull requests are run through travis-ci for continuous integration. You can avoid
+build failures by running the following checks before submitting your pull request:
+
+ make unittest
+ make lint
+ # The next two commands may change your code. It is recommended you commit
+ # before running them.
+ make clang-tidy # requires clang-tidy is installed
+ make format # requires clang-format is installed
+
+Note that the clang-tidy target may take a while to run. You might consider
+running clang-tidy separately on the files you have added/changed before
+invoking the make target to reduce iteration time.

cpp/build-support/run-clang-format.sh

@@ -0,0 +1,42 @@
+#!/bin/bash
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Runs clang format in the given directory
+# Arguments:
+# $1 - Path to the source tree
+# $2 - Path to the clang format binary
+# $3 - Apply fixes (will raise an error if false and not there where changes)
+# $ARGN - Files to run clang format on
+#
+SOURCE_DIR=$1
+shift
+CLANG_FORMAT=$1
+shift
+APPLY_FIXES=$1
+shift
+
+# clang format will only find its configuration if we are in 
+# the source tree or in a path relative to the source tree
+pushd $SOURCE_DIR
+if [ "$APPLY_FIXES" == "1" ]; then
+ $CLANG_FORMAT -i $@
+else
+
+ NUM_CORRECTIONS=`$CLANG_FORMAT -output-replacements-xml $@ | grep offset | wc -l`
+ if [ "$NUM_CORRECTIONS" -gt "0" ]; then
+ echo "clang-format suggested changes, please run 'make format'!!!!"
+ exit 1
+ fi
+fi 
+popd

cpp/build-support/run-clang-tidy.sh

@@ -0,0 +1,40 @@
+#!/bin/bash
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+#
+# Runs clang format in the given directory
+# Arguments:
+# $1 - Path to the clang tidy binary
+# $2 - Path to the compile_commands.json to use
+# $3 - Apply fixes (will raise an error if false and not there where changes)
+# $ARGN - Files to run clang-tidy on
+#
+CLANG_TIDY=$1
+shift
+COMPILE_COMMANDS=$1
+shift
+APPLY_FIXES=$1
+shift
+
+# clang format will only find its configuration if we are in 
+# the source tree or in a path relative to the source tree
+if [ "$APPLY_FIXES" == "1" ]; then
+ $CLANG_TIDY -p $COMPILE_COMMANDS -fix $@
+else
+ NUM_CORRECTIONS=`$CLANG_TIDY -p $COMPILE_COMMANDS $@ 2>&1 | grep -v Skipping | grep "warnings* generated" | wc -l`
+ if [ "$NUM_CORRECTIONS" -gt "0" ]; then
+ echo "clang-tidy had suggested fixes. Please fix these!!!"
+ exit 1
+ fi
+fi 

cpp/cmake_modules/FindClangTools.cmake

@@ -0,0 +1,60 @@
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Tries to find the clang-tidy and clang-format modules
+#
+# Usage of this module as follows:
+#
+# find_package(ClangTools)
+#
+# Variables used by this module, they can change the default behaviour and need
+# to be set before calling find_package:
+#
+# ClangToolsBin_HOME -
+# When set, this path is inspected instead of standard library binary locations
+# to find clang-tidy and clang-format
+#
+# This module defines
+# CLANG_TIDY_BIN, The path to the clang tidy binary
+# CLANG_TIDY_FOUND, Whether clang tidy was found
+# CLANG_FORMAT_BIN, The path to the clang format binary 
+# CLANG_TIDY_FOUND, Whether clang format was found
+
+find_program(CLANG_TIDY_BIN 
+ NAMES clang-tidy-3.8 clang-tidy-3.7 clang-tidy-3.6 clang-tidy
+ PATHS ${ClangTools_PATH} $ENV{CLANG_TOOLS_PATH} /usr/local/bin /usr/bin 
+ NO_DEFAULT_PATH
+)
+
+if ( "${CLANG_TIDY_BIN}" STREQUAL "CLANG_TIDY_BIN-NOTFOUND" ) 
+ set(CLANG_TIDY_FOUND 0)
+ message("clang-tidy not found")
+else()
+ set(CLANG_TIDY_FOUND 1)
+ message("clang-tidy found at ${CLANG_TIDY_BIN}")
+endif()
+
+find_program(CLANG_FORMAT_BIN 
+ NAMES clang-format-3.8 clang-format-3.7 clang-format-3.6 clang-format
+ PATHS ${ClangTools_PATH} $ENV{CLANG_TOOLS_PATH} /usr/local/bin /usr/bin 
+ NO_DEFAULT_PATH
+)
+
+if ( "${CLANG_FORMAT_BIN}" STREQUAL "CLANG_FORMAT_BIN-NOTFOUND" ) 
+ set(CLANG_FORMAT_FOUND 0)
+ message("clang-format not found")
+else()
+ set(CLANG_FORMAT_FOUND 1)
+ message("clang-format found at ${CLANG_FORMAT_BIN}")
+endif()
+

cpp/src/.clang-format

@@ -0,0 +1,65 @@
+---
+Language: Cpp
+# BasedOnStyle: Google
+AccessModifierOffset: -1
+AlignAfterOpenBracket: false 
+AlignConsecutiveAssignments: false
+AlignEscapedNewlinesLeft: true
+AlignOperands: true
+AlignTrailingComments: true
+AllowAllParametersOfDeclarationOnNextLine: true
+AllowShortBlocksOnASingleLine: true 
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: Inline
+AllowShortIfStatementsOnASingleLine: true
+AllowShortLoopsOnASingleLine: false 
+AlwaysBreakAfterDefinitionReturnType: None
+AlwaysBreakBeforeMultilineStrings: true
+AlwaysBreakTemplateDeclarations: true
+BinPackArguments: true
+BinPackParameters: true 
+BreakBeforeBinaryOperators: None
+BreakBeforeBraces: Attach
+BreakBeforeTernaryOperators: true
+BreakConstructorInitializersBeforeComma: false
+ColumnLimit: 90 
+CommentPragmas: '^ IWYU pragma:'
+ConstructorInitializerAllOnOneLineOrOnePerLine: true
+ConstructorInitializerIndentWidth: 4
+ContinuationIndentWidth: 4
+Cpp11BracedListStyle: true
+DerivePointerAlignment: false 
+DisableFormat: false
+ExperimentalAutoDetectBinPacking: false
+ForEachMacros: [ foreach, Q_FOREACH, BOOST_FOREACH ]
+IndentCaseLabels: true
+IndentWidth: 2
+IndentWrappedFunctionNames: false
+KeepEmptyLinesAtTheStartOfBlocks: false
+MacroBlockBegin: ''
+MacroBlockEnd: ''
+MaxEmptyLinesToKeep: 1
+NamespaceIndentation: None
+ObjCBlockIndentWidth: 2
+ObjCSpaceAfterProperty: false
+ObjCSpaceBeforeProtocolList: false
+PenaltyBreakBeforeFirstCallParameter: 1000
+PenaltyBreakComment: 300
+PenaltyBreakFirstLessLess: 120
+PenaltyBreakString: 1000
+PenaltyExcessCharacter: 1000000
+PenaltyReturnTypeOnItsOwnLine: 200
+PointerAlignment: Left
+SpaceAfterCStyleCast: false
+SpaceBeforeAssignmentOperators: true
+SpaceBeforeParens: ControlStatements
+SpaceInEmptyParentheses: false
+SpacesBeforeTrailingComments: 2
+SpacesInAngles: false
+SpacesInContainerLiterals: true
+SpacesInCStyleCastParentheses: false
+SpacesInParentheses: false
+SpacesInSquareBrackets: false
+Standard: Cpp11
+TabWidth: 8
+UseTab: Never

cpp/src/.clang-tidy

@@ -0,0 +1,14 @@
+---
+Checks: 'clang-diagnostic-*,clang-analyzer-*,-clang-analyzer-alpha*,google-.*,modernize-.*,readablity-.*'
+HeaderFilterRegex: 'arrow/.*'
+AnalyzeTemporaryDtors: true
+CheckOptions: 
+ - key: google-readability-braces-around-statements.ShortStatementLines
+ value: '1'
+ - key: google-readability-function-size.StatementThreshold
+ value: '800'
+ - key: google-readability-namespace-comments.ShortNamespaceLines
+ value: '10'
+ - key: google-readability-namespace-comments.SpacesBeforeComments
+ value: '2'
+

cpp/src/arrow/api.h

@@ -37,4 +37,4 @@
 #include "arrow/util/memory-pool.h"
 #include "arrow/util/status.h"
 
-#endif // ARROW_API_H
+#endif  // ARROW_API_H

cpp/src/arrow/array-test.cc

@@ -33,15 +33,12 @@ namespace arrow {
 
 class TestArray : public ::testing::Test {
  public:
- void SetUp() {
- pool_ = default_memory_pool();
- }
+ void SetUp() { pool_ = default_memory_pool(); }
 
  protected:
  MemoryPool* pool_;
 };
 
-
 TEST_F(TestArray, TestNullCount) {
  auto data = std::make_shared<PoolBuffer>(pool_);
  auto null_bitmap = std::make_shared<PoolBuffer>(pool_);
@@ -53,22 +50,23 @@ TEST_F(TestArray, TestNullCount) {
  ASSERT_EQ(0, arr_no_nulls->null_count());
 }
 
-
 TEST_F(TestArray, TestLength) {
  auto data = std::make_shared<PoolBuffer>(pool_);
  std::unique_ptr<Int32Array> arr(new Int32Array(100, data));
  ASSERT_EQ(arr->length(), 100);
 }
 
 TEST_F(TestArray, TestIsNull) {
+ // clang-format off
  std::vector<uint8_t> null_bitmap = {1, 0, 1, 1, 0, 1, 0, 0,
  1, 0, 1, 1, 0, 1, 0, 0,
  1, 0, 1, 1, 0, 1, 0, 0,
  1, 0, 1, 1, 0, 1, 0, 0,
  1, 0, 0, 1};
+ // clang-format on
  int32_t null_count = 0;
  for (uint8_t x : null_bitmap) {
- if (x == 0) ++null_count;
+ if (x == 0) { ++null_count; }
  }
 
  std::shared_ptr<Buffer> null_buf = test::bytes_to_null_buffer(null_bitmap);
@@ -85,8 +83,6 @@ TEST_F(TestArray, TestIsNull) {
  }
 }
 
+TEST_F(TestArray, TestCopy) {}
 
-TEST_F(TestArray, TestCopy) {
-}
-
-} // namespace arrow
+} // namespace arrow