Skip to content

ci: fix clang-tidy #10496

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

Merged
htuch merged 5 commits into from
Mar 26, 2020
Merged

ci: fix clang-tidy #10496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45a6da1
ci: fix clang-tidy
lizan Mar 24, 2020
7fc592a
fix
lizan Mar 24, 2020
d08c5a7
Merge remote-tracking branch 'upstream/master' into tidy_fix
lizan Mar 25, 2020
e2303a3
fix command
lizan Mar 25, 2020
738c184
fix tidy
lizan Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 3 additions & 16 deletions .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,4 @@
Checks: 'abseil-*,
bugprone-*,
clang-analyzer-*,
Copy link
Member

@htuch htuch Mar 24, 2020

Choose a reason for hiding this comment

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

Are we regressing anything by eliminating these?

Copy link
Member Author

@lizan lizan Mar 24, 2020

Choose a reason for hiding this comment

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

Those are checked but not enforced before but I don't think anyone is looking at it. So just check what we are enforcing. Then all errors are written to the fix yaml file so we can check in bash easily.

Copy link
Member

@mattklein123 mattklein123 Mar 24, 2020

Choose a reason for hiding this comment

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

Once we get this fixed is it possible to do a complete master run and fix all the regressions? I'm sure there are a lot. Thanks for doing this. Very excited to get this fixed!

clang-diagnostic-*,
misc-unused-using-decls,
modernize-*,
-modernize-pass-by-value,
-modernize-use-trailing-return-type,
performance-*,
readability-braces-around-statements,
readability-container-size-empty,
readability-identifier-naming,
readability-redundant-*'

#TODO(lizan): grow this list, fix possible warnings and make more checks as error
WarningsAsErrors: 'abseil-duration-*,
Checks: 'abseil-duration-*,
abseil-faster-strsplit-delimiter,
abseil-no-namespace,
abseil-redundant-strcat-calls,
Expand Down Expand Up @@ -51,6 +36,8 @@ WarningsAsErrors: 'abseil-duration-*,
readability-redundant-smartptr-get,
readability-redundant-string-cstr'

WarningsAsErrors: '*'

CheckOptions:
- key: bugprone-assert-side-effect.AssertMacros
value: 'ASSERT'
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ TAGS
.vimrc
.vs
.vscode
clang-tidy-fixes.yaml
clang.bazelrc
user.bazelrc
CMakeLists.txt
Expand Down
21 changes: 16 additions & 5 deletions ci/run_clang_tidy.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export LLVM_CONFIG=${LLVM_CONFIG:-llvm-config}
LLVM_PREFIX=${LLVM_PREFIX:-$(${LLVM_CONFIG} --prefix)}
CLANG_TIDY=${CLANG_TIDY:-$(${LLVM_CONFIG} --bindir)/clang-tidy}
CLANG_APPLY_REPLACEMENTS=${CLANG_APPLY_REPLACEMENTS:-$(${LLVM_CONFIG} --bindir)/clang-apply-replacements}
FIX_YAML=clang-tidy-fixes.yaml

# Quick syntax check of .clang-tidy.
${CLANG_TIDY} -dump-config > /dev/null 2> clang-tidy-config-errors.txt
Expand Down Expand Up @@ -57,20 +58,30 @@ function filter_excludes() {

if [[ "${RUN_FULL_CLANG_TIDY}" == 1 ]]; then
echo "Running full clang-tidy..."
"${LLVM_PREFIX}/share/clang/run-clang-tidy.py" \
python3 "${LLVM_PREFIX}/share/clang/run-clang-tidy.py" \
-clang-tidy-binary=${CLANG_TIDY} \
-clang-apply-replacements-binary=${CLANG_APPLY_REPLACEMENTS} \
-export-fixes=${FIX_YAML} \
-j ${NUM_CPUS:-0} -p 1 -quiet \
${APPLY_CLANG_TIDY_FIXES:+-fix}
elif [[ "${BUILD_REASON}" != "PullRequest" ]]; then
echo "Running clang-tidy-diff against previous commit..."
git diff HEAD^ | filter_excludes | \
"${LLVM_PREFIX}/share/clang/clang-tidy-diff.py" \
python3 "${LLVM_PREFIX}/share/clang/clang-tidy-diff.py" \
-clang-tidy-binary=${CLANG_TIDY} \
-p 1
-export-fixes=${FIX_YAML} \
-j ${NUM_CPUS:-0} -p 1 -quiet
else
echo "Running clang-tidy-diff against master branch..."
git diff "remotes/origin/${SYSTEM_PULLREQUEST_TARGETBRANCH}" | filter_excludes | \
"${LLVM_PREFIX}/share/clang/clang-tidy-diff.py" \
python3 "${LLVM_PREFIX}/share/clang/clang-tidy-diff.py" \
-clang-tidy-binary=${CLANG_TIDY} \
-p 1
-export-fixes=${FIX_YAML} \
-j ${NUM_CPUS:-0} -p 1 -quiet
fi

if [[ -s "${FIX_YAML}" ]]; then
echo "clang-tidy check failed, potentially fixed by clang-apply-replacements:"
cat ${FIX_YAML}
exit 1
Copy link
Member

@derekargueta derekargueta Mar 24, 2020

Choose a reason for hiding this comment

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

nice workaround to the non-informative exit code

fi
3 changes: 2 additions & 1 deletion test/integration/fake_upstream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ void FakeStream::encodeHeaders(const Http::HeaderMap& headers, bool end_stream)
headers_copy->addCopy(Http::LowerCaseString("x-served-by"),
parent_.connection().localAddress()->asString());
}

parent_.connection().dispatcher().post([this, headers_copy, end_stream]() -> void {
encoder_.encodeHeaders(*headers_copy, end_stream);
});
Expand All @@ -107,7 +108,7 @@ void FakeStream::encodeData(uint64_t size, bool end_stream) {
}

void FakeStream::encodeData(Buffer::Instance& data, bool end_stream) {
std::shared_ptr<Buffer::Instance> data_copy(new Buffer::OwnedImpl(data));
std::shared_ptr<Buffer::Instance> data_copy = std::make_shared<Buffer::OwnedImpl>(data);
parent_.connection().dispatcher().post(
[this, data_copy, end_stream]() -> void { encoder_.encodeData(*data_copy, end_stream); });
}
Expand Down