[go] net: eliminate bounds checks in hasUpperCase
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
net: eliminate bounds checks in hasUpperCase
Function hasUpperCase accesses its parameter's bytes while iterating over
its runes. This approach muddles intent and causes an unnecessary bounds
check.
This CL makes hasUpperCase iterate over its parameter's bytes instead.
Updates #76354
Change diff
diff --git a/src/net/parse.go b/src/net/parse.go
index 106a303d..7495ccf 100644
--- a/src/net/parse.go
+++ b/src/net/parse.go
@@ -182,8 +182,8 @@
// hasUpperCase tells whether the given string contains at least one upper-case.
func hasUpperCase(s string) bool {
- for i := range s {
- if 'A' <= s[i] && s[i] <= 'Z' {
+ for _, b := range []byte(s) {
+ if 'A' <= b && b <= 'Z' {
return true
}
}
Change information
- M src/net/parse.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 1 comment![]( "Open in Gerrit")
I think the current code is fine, though it could use a comment.
If we're going to change it I think it would be clearer to write
for i := range s
rather than the explicit conversion to []byte, and it could still use a comment.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
I think the current code is fine, though it could use a comment.
If we're going to change it I think it would be clearer to write
for i := range srather than the explicit conversion to []byte, and it could still use a comment.
I'm not following you...
```
for i := range s
```
is the current code.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
jub0bsI think the current code is fine, though it could use a comment.
If we're going to change it I think it would be clearer to write
for i := range srather than the explicit conversion to []byte, and it could still use a comment.
I'm not following you...
```
for i := range s
```is the current code.
Did you mean
```
for _, r := range s
```
perhaps? If so, I'm not sure why we should pay the price for decoding runes here.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 1 comment![]( "Open in Gerrit")
jub0bsI think the current code is fine, though it could use a comment.
If we're going to change it I think it would be clearer to write
for i := range srather than the explicit conversion to []byte, and it could still use a comment.
jub0bsI'm not following you...
```
for i := range s
```is the current code.
Did you mean
```
for _, r := range s
```perhaps? If so, I'm not sure why we should pay the price for decoding runes here.
Whoops, apologies, I meant to write
For I := range len(s)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
jub0bsI think the current code is fine, though it could use a comment.
If we're going to change it I think it would be clearer to write
for i := range srather than the explicit conversion to []byte, and it could still use a comment.
jub0bsI'm not following you...
```
for i := range s
```is the current code.
Ian Lance TaylorDid you mean
```
for _, r := range s
```perhaps? If so, I'm not sure why we should pay the price for decoding runes here.
Whoops, apologies, I meant to write
For I := range len(s)
Ok. I'm happy with either with either approach.
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Mengué (Gerrit)
Olivier Mengué voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Martí (Gerrit)
Daniel Martí voted Code-Review+2![]( "Open in Gerrit")
| Code-Review | +2 |
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jorropo
| Auto-Submit | +1 |
| Code-Review | +2 |
1 comment
for i := range len(s) {Your patch looks ridiculous.
I'll submit a CL to fix the compiler. This looks silly (it's not, due to how `range string(s)` is implemented but still looks silly).
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
for i := range len(s) {Your patch looks ridiculous.
I'll submit a CL to fix the compiler. This looks silly (it's not, due to how `range string(s)` is implemented but still looks silly).
I'm not sure how to respond to that... But if it can be fixed at the compiler level, I'm all for it.
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Martí (Gerrit)
Daniel Martí added 1 comment![]( "Open in Gerrit")
for i := range len(s) {jub0bsYour patch looks ridiculous.
I'll submit a CL to fix the compiler. This looks silly (it's not, due to how `range string(s)` is implemented but still looks silly).
I'm not sure how to respond to that... But if it can be fixed at the compiler level, I'm all for it.
I hope Jorropo just means that it should be unnecessary as long as we fix the compiler. But I still think that the new code is slightly better. You're looping over the bytes, not the characters. So IMHO we still want to merge this regardless.
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo added 1 comment![]( "Open in Gerrit")
for i := range len(s) {jub0bsYour patch looks ridiculous.
I'll submit a CL to fix the compiler. This looks silly (it's not, due to how `range string(s)` is implemented but still looks silly).
Daniel MartíI'm not sure how to respond to that... But if it can be fixed at the compiler level, I'm all for it.
I hope Jorropo just means that it should be unnecessary as long as we fix the compiler. But I still think that the new code is slightly better. You're looping over the bytes, not the characters. So IMHO we still want to merge this regardless.
@mv...@mvdan.cc you're impressively good at Jorropo to English translations.
@jub0bsin...@gmail.com this wasn't anything you had to respond to 😊
I was basically saying thanks for finding a new compiler test case.
Related details
- Ian Lance Taylor
- jub0bs
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Martí (Gerrit)
Daniel Martí added 1 comment![]( "Open in Gerrit")
for i := range len(s) {jub0bsYour patch looks ridiculous.
I'll submit a CL to fix the compiler. This looks silly (it's not, due to how `range string(s)` is implemented but still looks silly).
Daniel MartíI'm not sure how to respond to that... But if it can be fixed at the compiler level, I'm all for it.
JorropoI hope Jorropo just means that it should be unnecessary as long as we fix the compiler. But I still think that the new code is slightly better. You're looping over the bytes, not the characters. So IMHO we still want to merge this regardless.
@mv...@mvdan.cc you're impressively good at Jorropo to English translations.
@jub0bsin...@gmail.com this wasn't anything you had to respond to 😊
I was basically saying thanks for finding a new compiler test case.
@jorro...@gmail.com did you end up raising a CL or issue about the compiler improvement? if you did then I think we can abandon this; if it's not feasible then I think we should submit this.
Related details
- Ian Lance Taylor
- jub0bs
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
for i := range len(s) {jub0bsYour patch looks ridiculous.
I'll submit a CL to fix the compiler. This looks silly (it's not, due to how `range string(s)` is implemented but still looks silly).
Daniel MartíI'm not sure how to respond to that... But if it can be fixed at the compiler level, I'm all for it.
JorropoI hope Jorropo just means that it should be unnecessary as long as we fix the compiler. But I still think that the new code is slightly better. You're looping over the bytes, not the characters. So IMHO we still want to merge this regardless.
Daniel Martí@mv...@mvdan.cc you're impressively good at Jorropo to English translations.
@jub0bsin...@gmail.com this wasn't anything you had to respond to 😊
I was basically saying thanks for finding a new compiler test case.
@jorro...@gmail.com did you end up raising a CL or issue about the compiler improvement? if you did then I think we can abandon this; if it's not feasible then I think we should submit this.
I'm inclined to agree with @mv...@mvdan.cc. Future compiler improvements may well render this change unnecessary, but iterating over bytes rather than over runes makes more sense in this instance anyway.
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Martí (Gerrit)
Daniel Martí voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |