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 2 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-*,

@htuch htuch Mar 24, 2020

Copy link
Copy Markdown
Member

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?

@lizan lizan Mar 24, 2020

Copy link
Copy Markdown
Member Author

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.

@mattklein123 mattklein123 Mar 24, 2020

Copy link
Copy Markdown
Member

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
19 changes: 15 additions & 4 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 @@ -60,17 +61,27 @@ if [[ "${RUN_FULL_CLANG_TIDY}" == 1 ]]; then
"${LLVM_PREFIX}/share/clang/run-clang-tidy.py" \

@marcomagdy marcomagdy Mar 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

python3 here as well?

-clang-tidy-binary=${CLANG_TIDY} \
-clang-apply-replacements-binary=${CLANG_APPLY_REPLACEMENTS} \
-export-fixes=tidy.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

@derekargueta derekargueta Mar 24, 2020

Copy link
Copy Markdown
Member

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
6 changes: 3 additions & 3 deletions test/integration/fake_upstream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ void FakeStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers
void FakeStream::encodeHeaders(const Http::HeaderMap& headers, bool end_stream) {
std::shared_ptr<Http::ResponseHeaderMap> headers_copy(
Http::createHeaderMap<Http::ResponseHeaderMapImpl>(headers));
if (add_served_by_header_) {
if (add_served_by_header_)

@htuch htuch Mar 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this style change? I thought we had mandatory braces for single line if. I think Heartbleed or similar was caused by not doing this.

@lizan lizan Mar 24, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, I'm trying to prove clang-tidy will fail on this, and revert later.

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 +107,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