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.

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

clang-tidy: check casing on class names #8411

Merged

Conversation

derekargueta
Copy link
Member

Description: verify CamelCase for class names.
Risk Level: low
Testing: existing
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Derek Argueta [email protected]

@@ -101,7 +101,7 @@ TEST(Logger, checkLoggerLevel) {
}
};

logTestClass testObj;
LogTestClass testObj;
Copy link
Member

Choose a reason for hiding this comment

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

s/testObj/test_obj/ I think you'll catch this in next identifier-naming PR but feel free to fix here too.

@derekargueta
Copy link
Member Author

derekargueta commented Oct 1, 2019

I might need to revisit this check another time - clang-tidying source/common/common/fmt.h in isolation has an issue with #include "absl/strings/string_view.h" because the bazel target is an envoy_basic_cc_library, a thin wrapper around native.cc_library which doesn't know of abseil. Trying to move it to an envoy_cc_library causes a circular dependency ¯\_(ツ)_/¯

This isn't an effect of clang-tidy, just an issue in the build system that seems to have relied on transitive dependencies for getting abseil.

@lizan
Copy link
Member

lizan commented Oct 1, 2019

@derekargueta I think you may add abseil_strings deps to envoy_basic_cc_library, that shouldn't introduce circular dependency.

@lizan lizan merged commit 0360afe into envoyproxy:master Oct 4, 2019
@derekargueta derekargueta deleted the dereka/clang-tidy-identifier-class-name branch October 5, 2019 07:55
nandu-vinodan pushed a commit to nandu-vinodan/envoy that referenced this pull request Oct 17, 2019
Description: verify `CamelCase` for class names.
Risk Level: low
Testing: existing
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Derek Argueta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants