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

format: upgrade to clang-format-5.0 #1212

Merged
merged 4 commits into from
Jul 5, 2017
Merged

Conversation

mattklein123
Copy link
Member

This most enforces namespace comments but also churns/fixes a few
other things.

Fixes #1200
Fixes #673

This most enforces namespace comments but also churns/fixes a few
other things.

Fixes #1200
Fixes #673
@mattklein123
Copy link
Member Author

@lyft/network-team @htuch

htuch
htuch previously approved these changes Jul 5, 2017
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM

Http::CodeUtility::ResponseStatInfo info{config_->scope(), cluster_->statsScope(), EMPTY_STRING,
*getTooManyRequestsHeader(), true, EMPTY_STRING,
EMPTY_STRING, EMPTY_STRING, EMPTY_STRING, false};
Http::CodeUtility::ResponseStatInfo info{config_->scope(),
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this style of overflow for initializers? Seems it would make sense to have them match function argument style, where I think we are packing as much as we can into the line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I agree. Let me play with it a bit. There is something broken with header ordering also. I think I need to disable the header ordering feature. Let me play with it and I will tag you again.

Copy link
Member

Choose a reason for hiding this comment

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

Best to disable the header ordering in clang-format. I recall looking into whether we could get it to do what we wanted and concluded no. See #793. I think it had to do with the block splitting etc. Might be better in 5.0, not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm trying to figure out how to disable it in clang-format

Copy link
Member Author

Choose a reason for hiding this comment

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

@htuch AFAICT all bin packing options are enabled. I'm not sure this is configurable. I will keep poking around but do you know of which option I should be looking at?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked around and I can't find an obvious way to fix this, unless we change AlignAfterOpenBracket to DontAlign which changes a whole bunch of other things. Will leave for now and we can see if we can change this later. I agree it would be nice if it were like a normal function call.

RomanDzhabarov
RomanDzhabarov previously approved these changes Jul 5, 2017
htuch
htuch previously approved these changes Jul 5, 2017
@mattklein123 mattklein123 dismissed stale reviews from htuch and RomanDzhabarov via 690fc66 July 5, 2017 17:47
@mattklein123 mattklein123 merged commit 0efa18c into master Jul 5, 2017
@mattklein123 mattklein123 deleted the clang-format-upgrade branch July 5, 2017 18:34
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
Automatic merge from submit-queue.

Add the allow_missing_or_failed option to jwt-auth

**What this PR does / why we need it**: Add the allow_missing_or_failed option to jwt-auth so jwt-auth won't return 401 when jwt is missing or the jwt authentication fails.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#1212 

**Special notes for your reviewer**:

**Release note**:

```release-note
```
jpsim pushed a commit that referenced this pull request Nov 28, 2022
…sentation (#1212)

Description: Splits list-formatted header value on creation of header maps.  Per RFC 7230, does not perform this parsing on "set-cookie" headers, and additionally skips it on "www-authenticate" and "proxy-authenticate", due to their potential to contain commas in contravention of the spec.
Risk Level: Low
Testing: Pending

Signed-off-by: Mike Schore <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
…dd specific logic to the RetryPolicy classes (#1752)

Description: reverts earlier logic that splits comma-separated headers introduced in #1212 #1213. Upon further inspection of the Envoy code we now understand that the Envoy behavior to coalesce headers with multiple appearances in the header map is limited to Envoy's inline headers. Thus this PR reverts the Envoy Mobile logic to split up headers which should have no semantic impact in headers per RFC. However, we do add logic to the Envoy Mobile RetryPolicy classes which need individual values in order to correctly map to the swift/kotlin enum cases specific to those classes.
Risk Level: med - changes header handling logic
Testing: added unit tests

Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
…sentation (#1212)

Description: Splits list-formatted header value on creation of header maps.  Per RFC 7230, does not perform this parsing on "set-cookie" headers, and additionally skips it on "www-authenticate" and "proxy-authenticate", due to their potential to contain commas in contravention of the spec.
Risk Level: Low
Testing: Pending

Signed-off-by: Mike Schore <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
…dd specific logic to the RetryPolicy classes (#1752)

Description: reverts earlier logic that splits comma-separated headers introduced in #1212 #1213. Upon further inspection of the Envoy code we now understand that the Envoy behavior to coalesce headers with multiple appearances in the header map is limited to Envoy's inline headers. Thus this PR reverts the Envoy Mobile logic to split up headers which should have no semantic impact in headers per RFC. However, we do add logic to the Envoy Mobile RetryPolicy classes which need individual values in order to correctly map to the swift/kotlin enum cases specific to those classes.
Risk Level: med - changes header handling logic
Testing: added unit tests

Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: JP Simard <[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.

Upgrade to clang-format 5.0 Make namespace end comments more consistent
3 participants