Skip to content

common: fix errors from -Wrange-loop-analysis rule#13140

Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
rebello95:range-loop-fixes
Sep 17, 2020
Merged

common: fix errors from -Wrange-loop-analysis rule#13140
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
rebello95:range-loop-fixes

Conversation

@rebello95
Copy link
Copy Markdown
Contributor

@rebello95 rebello95 commented Sep 16, 2020 •
edited
Loading

With Xcode 12, -Wrange-loop-analysis becomes active and yields errors preventing Envoy Mobile from building. This PR fixes the errors in Envoy code yielded by this rule.

Unfortunately, I was unable to enable this rule in Envoy's build because of third party violations, but that is now being tracked in #13154.

Fixes #13139.

Risk Level: Low
Testing: Existing unit tests

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

@rebello95
common: enable -Wrange-loop-analysis and fix errors
473989d 
Fixes envoyproxy#13139.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
This was referenced Sep 16, 2020
Closed
Closed
@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 16, 2020

CI failure looks unrelated:

Updated Homebrew from 484c2b4fa to 871abe8b9.
Error: 767: unexpected token at '{
  "java11": "homebrew/core",
}
'

@rebello95
Merge remote-tracking branch 'origin/master' into range-loop-fixes
4ecb80a 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 17, 2020

cc @yanavlasov @lizan based on context in #11949

rebello95 added a commit to envoyproxy/envoy-mobile that referenced this pull request Sep 17, 2020
@rebello95
build: update to Xcode 12
a38606d 
This change is dependent on envoyproxy/envoy#13140 to fix upstream build failures with Xcode 12.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
rebello95 added a commit to envoyproxy/envoy-mobile that referenced this pull request Sep 17, 2020
@rebello95
build: update to Xcode 12
d948752 
This change is dependent on envoyproxy/envoy#13140 to fix upstream build failures with Xcode 12.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95 rebello95 mentioned this pull request Sep 17, 2020
Merged
@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 17, 2020

Hmm, it looks like there are some external violations:

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.

Is there a way to apply this rule only for Envoy code or do we need to fix each dependency?

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 17, 2020

cc @danzh2010. We will need to fix QUICHE also since ultimately xcode will end up complaining there also.

@rebello95
revert build rule
d4f76ec 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95 rebello95 mentioned this pull request Sep 17, 2020
Closed
@rebello95 rebello95 changed the title common: enable -Wrange-loop-analysis and fix errors common: fix errors from -Wrange-loop-analysis rule Sep 17, 2020
@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 17, 2020 •
edited
Loading

Looks like Envoy Mobile does not depend on QUICHE code right now, so the fixes in this PR are sufficient for getting Envoy Mobile to compile again. I dropped the build rule change and instead filed an issue to enable that separately, which should allow this PR to go green: #13154

@mattklein123 mind taking another look?

@mattklein123 mattklein123 self-assigned this Sep 17, 2020
mattklein123
mattklein123 approved these changes Sep 17, 2020
View reviewed changes
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!

@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 17, 2020

I believe remaining CI failures are unrelated

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Sep 17, 2020

Please merge main to fix CI.

/wait

@rebello95
Merge remote-tracking branch 'origin/master' into range-loop-fixes
2042a4d 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@mattklein123 mattklein123 merged commit ddfe3d7 into envoyproxy:master Sep 17, 2020
@rebello95 rebello95 deleted the range-loop-fixes branch September 17, 2020 23:28
rebello95 added a commit to envoyproxy/envoy-mobile that referenced this pull request Sep 18, 2020
@rebello95
build: update to Xcode 12 (#1106)
07ade35 
This change is dependent on envoyproxy/envoy#13140 to fix upstream build failures with Xcode 12.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
@moderation
Copy link
Copy Markdown
Contributor

moderation commented Sep 18, 2020

In addition the quiche issues the following is required to build in MacOS

diff --git a/source/extensions/filters/http/common/compressor/compressor.cc b/source/extensions/filters/http/common/compressor/compressor.cc
index 4e0a1b48c..e8e604295 100644
--- a/source/extensions/filters/http/common/compressor/compressor.cc
+++ b/source/extensions/filters/http/common/compressor/compressor.cc
@@ -224,7 +224,7 @@ CompressorFilter::chooseEncoding(const Http::ResponseHeaderMap& headers) const {
   }

   // Find all encodings accepted by the user agent and adjust the list of allowed compressors.
-  for (const auto token : StringUtil::splitToken(*accept_encoding_, ",", false /* keep_empty */)) {
+  for (const auto& token : StringUtil::splitToken(*accept_encoding_, ",", false /* keep_empty */)) {
     EncPair pair =
         std::make_pair(StringUtil::trim(StringUtil::cropRight(token, ";")), static_cast<float>(1));
     const auto params = StringUtil::cropLeft(token, ";");

@rebello95
Copy link
Copy Markdown
Contributor Author

rebello95 commented Sep 18, 2020

Thanks @moderation - added that as a task for #13154

rebello95 added a commit to rebello95/envoy that referenced this pull request Sep 18, 2020
@rebello95
build: enable -Wrange-loop-analysis rule
0e15eb9 
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>
This was referenced Sep 18, 2020
Closed
Closed
jpsim pushed a commit that referenced this pull request Nov 28, 2022
@rebello95 @jpsim
build: update to Xcode 12 (#1106)
65ef3db 
This change is dependent on #13140 to fix upstream build failures with Xcode 12.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
@rebello95 @jpsim
build: update to Xcode 12 (#1106)
877aa64 
This change is dependent on #13140 to fix upstream build failures with Xcode 12.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

error: building with Xcode 12 yields failures from -Wrange-loop-analysis

3 participants

@rebello95 @mattklein123 @moderation