Skip to content

build: enable -Wrange-loop-analysis rule#13184

Closed
rebello95 wants to merge 5 commits intoenvoyproxy:masterfrom
rebello95:range-loop-enable
Closed

build: enable -Wrange-loop-analysis rule#13184
rebello95 wants to merge 5 commits intoenvoyproxy:masterfrom
rebello95:range-loop-enable

Conversation

@rebello95
Copy link
Copy Markdown
Contributor

@rebello95 rebello95 commented Sep 18, 2020

Xcode 12 builds with this enabled, so adding this to Envoy will ensure compatibility and prevent breakages in Envoy Mobile's iOS build.

Resolves #13154.

Risk Level: Low
Testing: Existing unit tests

Signed-off-by: Michael Rebello me@michaelrebello.com

Xcode 12 builds with this enabled, so adding this to Envoy will ensure compatibility and prevent breakages in Envoy Mobile's iOS build.

Resolves envoyproxy#13140.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
mattklein123
mattklein123 previously approved these changes Sep 18, 2020
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 self-assigned this Sep 18, 2020
@yanavlasov
Copy link
Copy Markdown
Contributor

Looks like the build is failing because of the new option.

@yanavlasov yanavlasov self-assigned this Sep 18, 2020
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Copy Markdown
Contributor Author

Looks like we are still getting errors from absl. Will probably need to wait until those are fixed upstream (or is there a way to only apply -Wrange-loop-analysis to Envoy's code?)

external/com_google_absl/absl/strings/internal/str_split_internal.h:404:23: error: loop variable 'sp' of type 'const std::__1::basic_string_view<char, std::__1::char_traits<char> >' creates a copy from type 'const std::__1::basic_string_view<char, std::__1::char_traits<char> >' [-Werror,-Wrange-loop-analysis]
      for (const auto sp : splitter) {
                      ^
external/com_google_absl/absl/strings/internal/str_split_internal.h:305:12: note: in instantiation of member function 'absl::strings_internal::Splitter<absl::ByChar, absl::SkipEmpty>::ConvertToContainer<std::__1::map<std::__1::basic_string<char>, std::__1::basic_string<char>, std::__1::less<std::__1::basic_string<char> >, std::__1::allocator<std::__1::pair<const std::__1::basic_string<char>, std::__1::basic_string<char> > > >, std::__1::pair<const std::__1::basic_string<char>, std::__1::basic_string<char> >, true>::operator()' requested here
    return ConvertToContainer<Container, typename Container::value_type,
           ^
source/extensions/filters/network/postgres_proxy/postgres_decoder.cc:360:17: note: in instantiation of function template specialization 'absl::strings_internal::Splitter<absl::ByChar, absl::SkipEmpty>::operator map<std::__1::map<std::__1::basic_string<char>, std::__1::basic_string<char>, std::__1::less<std::__1::basic_string<char> >, std::__1::allocator<std::__1::pair<const std::__1::basic_string<char>, std::__1::basic_string<char> > > >, void>' requested here
  attributes_ = absl::StrSplit(message_.substr(4), absl::ByChar('\0'), absl::SkipEmpty());
                ^
external/com_google_absl/absl/strings/internal/str_split_internal.h:404:12: note: use reference type 'const std::__1::basic_string_view<char, std::__1::char_traits<char> > &' to prevent copying
      for (const auto sp : splitter) {
           ^~~~~~~~~~~~~~~

@mattklein123
Copy link
Copy Markdown
Member

It's not going to be easily possible for headers like this. I would recommend opening an abseil issue and linking it to the tracking issue and not setting it here project wide.

@rebello95
Copy link
Copy Markdown
Contributor Author

Sounds good, I'll close this PR for now until abseil/abseil-cpp#787 is fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

build: enable -Wrange-loop-analysis

3 participants