Skip to content

Refactor Read function in malformed_host_override.go#39046

Closed
withlin wants to merge 3 commits intomoby:masterfrom
withlin:if
Closed

Refactor Read function in malformed_host_override.go#39046
withlin wants to merge 3 commits intomoby:masterfrom
withlin:if

Conversation

@withlin
Copy link
Copy Markdown
Contributor

@withlin withlin commented Apr 10, 2019

Signed-off-by: Fu JinLin withlin@yeah.net

  • What I did

Refactor the Read function.

  • How I did it

Reduce the number of ifs used

withlin added 2 commits April 10, 2019 14:34
Signed-off-by: Fu JinLin <withlin@yeah.net>
Signed-off-by: Fu JinLin <withlin@yeah.net>
continue
}
if b[i+7] != '/' {
if b[i+1] != 'H' || b[i+2] != 'o' || b[i+3] != 's' ||
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Apr 10, 2019

Choose a reason for hiding this comment

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

Although it's more compact, I don't think this makes it more readable than the existing code. Perhaps something like this would work;

Suggested change
if b[i+1] != 'H' || b[i+2] != 'o' || b[i+3] != 's' ||
if string(b[i:i+8]) != "\nHost: /" {

edit; fix off-by-one (last should be i+8, not i+7)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

Signed-off-by: Fu JinLin <withlin@yeah.net>
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d9d9ecc). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #39046   +/-   ##
=========================================
  Coverage          ?   36.91%           
=========================================
  Files             ?      612           
  Lines             ?    45327           
  Branches          ?        0           
=========================================
  Hits              ?    16732           
  Misses            ?    26308           
  Partials          ?     2287

@thaJeztah
Copy link
Copy Markdown
Member

Thank you for updating! The changes LGTM 🤗

However, after some discussion on #38928 (comment), we decided to remove this hack entirely (see #39076).

For that reason, I'll have to close this PR.

Thank you so much for contributing! Even though this PR won't be merged, it's really appreciated!

@thaJeztah thaJeztah closed this Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants