-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
ARROW-71: [C++] Add clang-tidy and clang-format to the the tool chain. #55
Changes from all commits
4dd0b81
26945e9
9c556ef
d3f76d8
e7723d1
169352f
2fafb10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
#!/bin/bash | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to add apache header |
||
# | ||
# 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,4 +37,4 @@ | |
#include "arrow/util/memory-pool.h" | ||
#include "arrow/util/status.h" | ||
|
||
#endif // ARROW_API_H | ||
#endif // ARROW_API_H |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,15 +33,12 @@ namespace arrow { | |
|
||
class TestArray : public ::testing::Test { | ||
public: | ||
void SetUp() { | ||
pool_ = default_memory_pool(); | ||
} | ||
void SetUp() { pool_ = default_memory_pool(); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's stick with class scope, then, for now. Thanks |
||
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to add apache header.