Skip to content

lint: add more linters for using absl:: over std::#13043

Merged
mattklein123 merged 10 commits intoenvoyproxy:masterfrom
rebello95:std-visit-linter
Sep 11, 2020
Merged

lint: add more linters for using absl:: over std::#13043
mattklein123 merged 10 commits intoenvoyproxy:masterfrom
rebello95:std-visit-linter

Conversation

@rebello95
Copy link
Contributor

@rebello95 rebello95 commented Sep 10, 2020 •
edited
Loading

Follow up to #13040. Some of these examples are based on absl docs and C++ docs.

These linters are here to prevent additional breakages of Envoy Mobile on iOS due to usages of std:: types/functions that are not supported on iOS 11. More discussion in #12341.

Risk Level: Low
Testing: In PR

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

@rebello95
lint: add linter for preferring absl::visit over std::visit
36bd809 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95 rebello95 mentioned this pull request Sep 10, 2020
Merged
@mk46
Copy link
Contributor

mk46 commented Sep 10, 2020

Sorry, format check fails and run ci/run_envoy_docker.sh 'ci/do_ci.sh fix_format' to fix it.

@htuch htuch self-assigned this Sep 10, 2020
@htuch
Copy link
Member

htuch commented Sep 10, 2020

LGTM modulo format fix.

@rebello95
precommit
8efeb95 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Contributor Author

rebello95 commented Sep 10, 2020

Thanks. Running the docker command didn't work for me but I fixed locally and pushed

ggreenway
ggreenway requested changes Sep 10, 2020
View reviewed changes
Copy link
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

I think we should add the entire (commonly used) API for variants, which unfortunately for this purpose are free functions, not members, so they all need a linter entry:

std::holds_alternative
std::get
std::monostate
std::get_if

@mattklein123
Copy link
Member

mattklein123 commented Sep 10, 2020

Also, while you are in here did we ever add a linter for string_view? We need that also.

@rebello95
more tests
202066d 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Merge remote-tracking branch 'origin/master' into std-visit-linter
1d3895e 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95 rebello95 changed the title lint: add linter for preferring absl::visit over std::visit lint: add more linters for using absl:: over std:: Sep 10, 2020
@rebello95
more updates
4b62380 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Update std_visit.cc
1b0744b 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Contributor Author

rebello95 commented Sep 10, 2020

Done, added additional linters / test cases.

ggreenway
ggreenway previously approved these changes Sep 10, 2020
View reviewed changes
Copy link
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Thank you!

@rebello95
Copy link
Contributor Author

rebello95 commented Sep 10, 2020

Hm, looks like there are a lot of usages of std::get in the codebase today, which is why the linter is failing. Are you sure we need that one @ggreenway?

@ggreenway
Copy link
Member

ggreenway commented Sep 10, 2020

Hm, looks like there are a lot of usages of std::get in the codebase today, which is why the linter is failing. Are you sure we need that one @ggreenway?

Huh, std::get is used for both tuples and variants. We want to lint for only the uses with variant, not tuple, probably. But that's well into the realm of needing clang-tidy. So for now, I'd say remove the std::get one.

@rebello95
remove std::get linter
a93b02a 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Contributor Author

rebello95 commented Sep 10, 2020

Sounds good, removed.

@rebello95
Copy link
Contributor Author

rebello95 commented Sep 10, 2020

Looks like we also have existing usages of string_view which don't seem to be breaking Envoy Mobile @mattklein123: https://dev.azure.com/cncf/envoy/_build/results?buildId=50922&view=logs&jobId=9fd74226-df5b-5f29-39da-76fc341cc763&j=9fd74226-df5b-5f29-39da-76fc341cc763&t=068b98b1-acf0-52d0-fc6e-f6d1a5b1d5ef

@mattklein123
Copy link
Member

mattklein123 commented Sep 10, 2020

I thought we had decided somehow that string_view doesn't work on Envoy Mobile? I'm confused. cc @junr03 @goaway

@junr03
Copy link
Member

junr03 commented Sep 11, 2020

@rebello95 looks like those uses are all in extensions code we don't include/compile into Envoy Mobile. Hence why they are in the codebase here but causes no trouble to Envoy Mobile.

But yes, the decision was to not use std::string_view, so the linter change here to prohibit it is great, thanks!

@rebello95
fix violations
7e1e2c4 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Merge remote-tracking branch 'origin/master' into std-visit-linter
c555728 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Contributor Author

rebello95 commented Sep 11, 2020

Thanks for confirming @junr03! I pushed a change to fix the violations from the main branch.

mattklein123
mattklein123 previously approved these changes Sep 11, 2020
View reviewed changes
Copy link
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.

Awesome, thank you!

@rebello95
linter
6514d61 
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@rebello95
Copy link
Contributor Author

rebello95 commented Sep 11, 2020

Coverage failure looks unrelated

@mattklein123
Copy link
Member

mattklein123 commented Sep 11, 2020

I think coverage on main branch is broken somehow due to recent limit changes. I will merge this and we can sort it out there.

@mattklein123 mattklein123 merged commit 48d739b into envoyproxy:master Sep 11, 2020
@rebello95 rebello95 deleted the std-visit-linter branch September 11, 2020 23:09
@rebello95
Copy link
Contributor Author

rebello95 commented Sep 11, 2020

Thanks!

lhluo pushed a commit to lhluo/envoy that referenced this pull request Sep 11, 2020
Merge remote-tracking branch 'upstream/master' into binary-lua-b64-en…
808c8f7 
…code

* upstream/master:
  lint: add more linters for using absl:: over std:: (envoyproxy#13043)
  udpa: filesystem list collection support for inline entries. (envoyproxy#13028)
  filter: http: jwt: implement matching for HTTP CONNECT (envoyproxy#13064)
  [fuzz] split http filter logic into a fuzzing class (envoyproxy#13016)
  xds: allow empty delta update (envoyproxy#12699)
  CacheFilter: parses the allowed_vary_headers from the cache config. (envoyproxy#12928)
  router: extend HTTP CONNECT route matching criteria (envoyproxy#13056)
  docs: clarify use of Extended CONNECT for h/2 (envoyproxy#13051)
  build: shellcheck tools/ (envoyproxy#13007)
  [fuzz] Refactored Health Checker Impl Tests (envoyproxy#13017)

Signed-off-by: Lihao Luo <lihao@squareup.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

@ggreenway ggreenway ggreenway left review comments

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@jmarantz jmarantz Awaiting requested review from jmarantz

Assignees

@htuch htuch

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rebello95 @mk46 @htuch @mattklein123 @ggreenway @junr03