build: enable -Wrange-loop-analysis

[rebello95]

Title: build: enable -Wrange-loop-analysis

Context:

In #13139 we noticed that building with Xcode 12 enables -Wrange-loop-analysis and yields errors preventing Envoy from building. More specifically, this affected Envoy Mobile when building for iOS. The code errors in Envoy code consumed by Envoy Mobile were fixed in #13140, but there are some third parties (not depended on by Envoy Mobile) that prevent us from enabling this build rule in Envoy.

For example:

bazel-out/darwin-fastbuild/bin/external/com_googlesource_quiche/quiche/spdy/core/hpack/hpack_header_table.cc:258:19: error: loop variable 'it' of type 'const std::__1::pair<const std::__1::basic_string_view<char, std::__1::char_traits<char> >, const spdy::HpackEntry *>' creates a copy from type 'const std::__1::pair<const std::__1::basic_string_view<char, std::__1::char_traits<char> >, const spdy::HpackEntry *>' [-Werror,-Wrange-loop-analysis]
  for (const auto it : static_name_index_) {
                  ^
bazel-out/darwin-fastbuild/bin/external/com_googlesource_quiche/quiche/spdy/core/hpack/hpack_header_table.cc:258:8: note: use reference type 'const std::__1::pair<const std::__1::basic_string_view<char, std::__1::char_traits<char> >, const spdy::HpackEntry *> &' to prevent copying
  for (const auto it : static_name_index_) {
       ^~~~~~~~~~~~~~~
bazel-out/darwin-fastbuild/bin/external/com_googlesource_quiche/quiche/spdy/core/hpack/hpack_header_table.cc:266:19: error: loop variable 'it' of type 'const std::__1::pair<const std::__1::basic_string_view<char, std::__1::char_traits<char> >, const spdy::HpackEntry *>' creates a copy from type 'const std::__1::pair<const std::__1::basic_string_view<char, std::__1::char_traits<char> >, const spdy::HpackEntry *>' [-Werror,-Wrange-loop-analysis]
  for (const auto it : dynamic_name_index_) {
                  ^
bazel-out/darwin-fastbuild/bin/external/com_googlesource_quiche/quiche/spdy/core/hpack/hpack_header_table.cc:266:8: note: use reference type 'const std::__1::pair<const std::__1::basic_string_view<char, std::__1::char_traits<char> >, const spdy::HpackEntry *> &' to prevent copying
  for (const auto it : dynamic_name_index_) {
       ^~~~~~~~~~~~~~~
2 errors generated.

Ask:

Enable the -Wrange-loop-analysis rule in Envoy for building once we're able to (after third parties are fixed) to avoid future build failures caused by this error in Envoy Mobile.

[asraa]

Some third parties could probably suppress this by a change their build rule. For QUICHE, can you add -Wno-range-loop-analysis here?

envoy/bazel/external/quiche.BUILD

Line 56 in 933e267

quiche_copts = select({

[rebello95]

Nice, thanks @asraa - looks like that's being added in #13154 by @danzh2010, after which I can submit a PR to add this rule to Envoy's build file

[rebello95]

As part of this, we'll need to fix: #13140 (comment)

[rebello95]

#13184

[rebello95]

Needs abseil/abseil-cpp#787 in abseil

[mattklein123]

cc @yanavlasov ^ who is tracking other compile issues internally with the abseil team.

[rebello95]

New violation that this would have caught: #13342

[moderation]

These are required to compile on MacOS with the latest xcode 12. Related #13139

diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc
index a7d674d5d..6c8afffbf 100644
--- a/source/common/http/header_map_impl.cc
+++ b/source/common/http/header_map_impl.cc
@@ -503,7 +503,7 @@ HeaderMap::NonConstGetResult HeaderMapImpl::getExisting(const LowerCaseString& k
     if (iter != headers_.mapEnd()) {
       const HeaderList::HeaderNodeVector& v = iter->second;
       ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value.
-      for (const auto values_it : v) {
+      for (const auto& values_it : v) {
         // Convert the iterated value to a HeaderEntry*.
         ret.push_back(&(*values_it));
       }
diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc
index 223774c39..b51687832 100644
--- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc
+++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc
@@ -351,7 +351,7 @@ ResponsePtr RawHttpClientImpl::toResponse(Http::ResponseMessagePtr message) {
         std::vector<absl::string_view> header_names = StringUtil::splitToken(
             storage_header_value, ",", /*keep_empty_string=*/false, /*trim_whitespace=*/true);
         headers_to_remove.reserve(headers_to_remove.size() + header_names.size());
-        for (const auto header_name : header_names) {
+        for (const auto& header_name : header_names) {
           headers_to_remove.push_back(Http::LowerCaseString(std::string(header_name)));
         }
       }

[mattklein123]

Can we just disable this warning in the OSX/iOS build for now? This seems a simpler short term solution. cc @keith

[keith]

+1 should be easy enough to drop into the bazelrc temporarily

[keith]

turns out it's not easy enough. the problem is that global --copt flags come before the flags defined in copts = [] on the cc_library targets. This is where envoy passes -Wall, so if you disable this warning flag, it gets re-enabled with the -Wall

[rebello95]

After discussing with @keith (results above ^), it looks like the "easiest" way to fix this is still by enabling the rule in Envoy. Attempting again now that abseil has fixed their violations: #13364