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

Some code improvements #1750

Merged
merged 1 commit into from
Jul 4, 2019
Merged

Some code improvements #1750

merged 1 commit into from
Jul 4, 2019

Conversation

sosiska
Copy link
Contributor

@sosiska sosiska commented Jun 14, 2019

  • Rewrite empty string checks more idiomatically.
  • Change strings.ToLower comparisons to strings.EqualFold.
  • Rewrite switch statement with only one case as if.

@boypt
Copy link
Contributor

boypt commented Jun 14, 2019

  • both empty string checks methods are idiomatic and generate same code by go.
  • strings.EqualFold is better though.

@xiaokangwang
Copy link
Contributor

You have bundled too many changes in a single commit and a single pull request. Some additional time is necessary for the maintenance team to analyze your change.

@sosiska
Copy link
Contributor Author

sosiska commented Jun 14, 2019

both empty string checks methods are idiomatic and generate same code by go.

Based on Russ Cox's comment in a golang-nuts topic:

if I care about "is it this specific string" I tend to write s == "".

Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

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

There is one change in this pull request needs additional attention.

I didn't find any other issues in this pull request.

@@ -83,7 +83,7 @@ func generateRandomBytes(random []byte, connType [4]byte) {
continue
}

if 0x00000000 == (uint32(random[7])<<24)|(uint32(random[6])<<16)|(uint32(random[5])<<8)|uint32(random[4]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The original coding style can be used to prevent accidental assignment. While this is redundant in this case, this practice should be considered to be a beneficial one. Additional justification required for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can reject it, if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rejected in 0401a91

* Rewrite empty string checks more idiomatically.
* Change strings.ToLower comparisons to strings.EqualFold.
* Rewrite switch statement with only one case as if.
@xiaokangwang xiaokangwang self-requested a review July 4, 2019 12:24
Copy link
Contributor

@xiaokangwang xiaokangwang 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 this pull request is ready to be merged.

@xiaokangwang xiaokangwang requested a review from kslr July 4, 2019 12:25
@kslr kslr merged commit 3deb5f2 into v2ray:master Jul 4, 2019
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.

None yet

4 participants