You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/01/05 05:13:14 UTC
[GitHub] [trafficserver] masaori335 opened a new pull request #8576: LGTM: Fix constant comparison
masaori335 opened a new pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576
https://lgtm.com/projects/g/apache/trafficserver/?mode=tree&ruleFocus=2154840804
I might be misunderstanding, but many of the alerts seem false positive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r779918733
##########
File path: src/traffic_layout/engine.cc
##########
@@ -227,7 +227,7 @@ LayoutEngine::create_runroot()
empty_check_directory = ts_runroot;
if (ftw(ts_runroot.c_str(), check_directory_empty, OPEN_MAX_FILE) != 0) {
// if the directory is not empty, check for valid Y/N
- for (int i = 0; i < 3; i++) {
+ for (int i = 0; i < 3; i++) { // lgtm[cpp/constant-comparison]
Review comment:
Right. This is just checking 'Y/N'...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r778973918
##########
File path: plugins/experimental/memcache/tsmemcache.cc
##########
@@ -514,7 +514,7 @@ MC::binary_get_event(int event, void *data)
} else {
return write_binary_error(PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, 0);
}
- } else if (event == CACHE_EVENT_OPEN_READ) {
+ } else if (event == CACHE_EVENT_OPEN_READ) { // lgtm[cpp/constant-comparison]
Review comment:
Assuming `CACHE_EVENT_OPEN_READ != TSMEMCACHE_EVENT_GOT_ITEM`, this code is unreachable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#issuecomment-1006998850
Addressed @ywkaras pointed out. Obviously, I was still in vacation mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r778972574
##########
File path: plugins/experimental/memcache/tsmemcache.cc
##########
@@ -503,7 +503,7 @@ MC::binary_get_event(int event, void *data)
key = binary_get_key(this);
header.nkey = binary_header.request.keylen;
return get_item();
- } else if (event == CACHE_EVENT_OPEN_READ_FAILED) {
+ } else if (event == CACHE_EVENT_OPEN_READ_FAILED) { // lgtm[cpp/constant-comparison]
Review comment:
Assuming `CACHE_EVENT_OPEN_READ_FAILED != TSMEMCACHE_EVENT_GOT_ITEM`, this code is unreachable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r779915405
##########
File path: iocore/net/UnixUDPNet.cc
##########
@@ -534,7 +534,7 @@ UDPNetProcessor::recvfrom_re(Continuation *cont, void *token, int fd, struct soc
cont->handleEvent(NET_EVENT_DATAGRAM_READ_COMPLETE, event);
completionUtil::destroy(event);
return ACTION_RESULT_DONE;
- } else if (actual == 0 || (actual < 0 && actual == -EAGAIN)) {
+ } else if (actual == 0 || (actual < 0 && actual == -EAGAIN)) { // lgtm[cpp/constant-comparison]
Review comment:
Ah, right. Now I got what the alert says.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r778988808
##########
File path: src/traffic_logstats/logstats.cc
##########
@@ -1108,7 +1108,7 @@ update_codes(OriginStats *stat, int code, int size)
update_counter(stat->codes.c_4xx, size);
} else if (code >= 300) {
update_counter(stat->codes.c_3xx, size);
- } else if (code >= 200) {
+ } else if (code >= 200) { // lgtm[cpp/constant-comparison]
Review comment:
This could be:
```
} else /* 200 <= code < 300 */ {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r779917758
##########
File path: plugins/experimental/memcache/tsmemcache.cc
##########
@@ -503,7 +503,7 @@ MC::binary_get_event(int event, void *data)
key = binary_get_key(this);
header.nkey = binary_header.request.keylen;
return get_item();
- } else if (event == CACHE_EVENT_OPEN_READ_FAILED) {
+ } else if (event == CACHE_EVENT_OPEN_READ_FAILED) { // lgtm[cpp/constant-comparison]
Review comment:
That's right! These `else if` blocks are dead code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 merged pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
masaori335 merged pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r778986235
##########
File path: src/traffic_layout/engine.cc
##########
@@ -227,7 +227,7 @@ LayoutEngine::create_runroot()
empty_check_directory = ts_runroot;
if (ftw(ts_runroot.c_str(), check_directory_empty, OPEN_MAX_FILE) != 0) {
// if the directory is not empty, check for valid Y/N
- for (int i = 0; i < 3; i++) {
+ for (int i = 0; i < 3; i++) { // lgtm[cpp/constant-comparison]
Review comment:
This can just be:
```
for (int i = 0; true; i++) {
```
because the if statement at line 240 guarantees that i will never be 3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r778966317
##########
File path: iocore/net/UnixUDPNet.cc
##########
@@ -989,7 +989,7 @@ UDPQueue::SendUDPPacket(UDPPacketInternal *p, int32_t /* pktLen ATS_UNUSED */)
while (true) {
// stupid Linux problem: sendmsg can return EAGAIN
n = ::sendmsg(p->conn->getFd(), &msg, 0);
- if ((n >= 0) || ((n < 0) && (errno != EAGAIN))) {
+ if ((n >= 0) || ((n < 0) && (errno != EAGAIN))) { // lgtm[cpp/constant-comparison]
Review comment:
```
A B A || B A || (!A && B)
- - ------ --------------
F F F F
F T T T
T F T T
T T T T
```
So the above is equivalent to:
```
if ((n >= 0) || ((errno != EAGAIN)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r779001985
##########
File path: src/traffic_manager/traffic_manager.cc
##########
@@ -179,7 +179,7 @@ is_server_idle_from_new_connection()
Debug("lm", "%" PRId64 " active clients, threshold is %" PRId64, active, threshold);
- return active <= threshold;
+ return true;
}
Review comment:
I suggest:
```
static bool
is_server_idle_from_new_connection()
{
RecInt threshold = 0;
Debug("lm", "%" PRId64 " active clients, threshold is %" PRId64, active, threshold);
#if 0
// TODO implement with the right metric
RecInt active = 0;
return active <= threshold;
#else
return true;
#endif
}
```
This is clearer, and will avoid any potential unused local variable warnings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r779924303
##########
File path: plugins/experimental/memcache/tsmemcache.cc
##########
@@ -498,45 +498,16 @@ int
MC::binary_get_event(int event, void *data)
{
ink_assert(!"EVENT_ITEM_GOT is incorrect here");
- if (event != TSMEMCACHE_EVENT_GOT_ITEM) {
- CHECK_READ_AVAIL(binary_header.request.keylen, &MC::binary_get);
- key = binary_get_key(this);
- header.nkey = binary_header.request.keylen;
- return get_item();
- } else if (event == CACHE_EVENT_OPEN_READ_FAILED) {
Review comment:
As @ywkaras pointed out, these `else if` blocks are unreachable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8576: LGTM: Fix constant comparison
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8576:
URL: https://github.com/apache/trafficserver/pull/8576#discussion_r778951223
##########
File path: iocore/net/UnixUDPNet.cc
##########
@@ -534,7 +534,7 @@ UDPNetProcessor::recvfrom_re(Continuation *cont, void *token, int fd, struct soc
cont->handleEvent(NET_EVENT_DATAGRAM_READ_COMPLETE, event);
completionUtil::destroy(event);
return ACTION_RESULT_DONE;
- } else if (actual == 0 || (actual < 0 && actual == -EAGAIN)) {
+ } else if (actual == 0 || (actual < 0 && actual == -EAGAIN)) { // lgtm[cpp/constant-comparison]
Review comment:
Seems like this should simply be:
```
} else if (actual == 0 || actual == -EAGAIN) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org