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

optimize(bytesconv): add support for go1.20 #686

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

a631807682
Copy link
Member

@a631807682 a631807682 commented Mar 24, 2023

What type of PR is this?

optimize

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.

(Optional) Translate the PR title into Chinese.

bytesconv 增加 go1.20 的支持

(Optional) More detail description for this PR(en: English/zh: Chinese).

en:
go1.20 provides a truly safe(ish) APIs people can rely on, to ensure that subsequent go versions will not break b2s/s2b (no real world examples). refer to
golang/go#53003
valyala/fasthttp#1481

zh(optional):
使用 go1.20 提供的可以依赖的 api 去保证后续 go 版本不会破坏b2s/s2b(没有真实场景,不急)。参考
golang/go#53003
valyala/fasthttp#1481

Which issue(s) this PR fixes:

@a631807682 a631807682 requested review from a team as code owners March 24, 2023 09:02
@@ -1,15 +1,28 @@
module github.com/cloudwego/hertz

go 1.16
go 1.20
Copy link
Member

Choose a reason for hiding this comment

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

em... this may be a problem - since we have plenty of projects running under 1.20

Copy link
Member Author

Choose a reason for hiding this comment

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

em... this may be a problem - since we have plenty of projects running under 1.20

This is just a precaution, currently LTS supported by go are 1.18-1.20, maybe we can keep this PR open without merging until necessary.

https://go.dev/doc/devel/release#policy

Copy link
Member

Choose a reason for hiding this comment

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

One more word, there are still many users using low versions of go and we need to be responsible for them.
For this reason we have not used io instead of ioutil and upgraded the x/sys version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, can I close it?

Copy link
Member

Choose a reason for hiding this comment

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

we can keep this PR open without merging until necessary.

@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.78 🎉

Comparison is base (c98aec7) 72.49% compared to head (13b7d8f) 74.28%.

❗ Current head 13b7d8f differs from pull request most recent head 50966b8. Consider uploading reports for the commit 50966b8 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #686      +/-   ##
===========================================
+ Coverage    72.49%   74.28%   +1.78%     
===========================================
  Files           96      100       +4     
  Lines         9392     9396       +4     
===========================================
+ Hits          6809     6980     +171     
+ Misses        2154     1985     -169     
- Partials       429      431       +2     
Impacted Files Coverage Δ
internal/bytesconv/bytesconv.go 89.71% <ø> (-1.04%) ⬇️
internal/bytesconv/b2s_new.go 100.00% <100.00%> (ø)
internal/bytesconv/b2s_old.go 100.00% <100.00%> (ø)
internal/bytesconv/s2b_new.go 100.00% <100.00%> (ø)
internal/bytesconv/s2b_old.go 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@welkeyever
Copy link
Member

Is there any problem that related to current impl with Go1.20?
For compatibility reasons, we currently may not change the minimum compatible version in go.mod🙏

@a631807682
Copy link
Member Author

Is there any problem that related to current impl with Go1.20?
For compatibility reasons, we currently may not change the minimum compatible version in go.mod🙏

unsafe.SliceData/unsafe.StringData supported by 1.20

internal/bytesconv/b2s_new.go:52:23: unsafe.SliceData requires go1.20 or later (-lang was set to go1.16; check go.mod)
internal/bytesconv/s2b_new.go:50:22: unsafe.StringData requires go1.20 or later (-lang was set to go1.16; check go.mod)

@li-jin-gou li-jin-gou added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants