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 2020/08/11 01:28:30 UTC
[GitHub] [trafficserver] randall opened a new pull request #7100: Fixes uninitialized variables found by Xcode
randall opened a new pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#issuecomment-673095150
[approve ci autest]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#discussion_r468681650
##########
File path: proxy/http/HttpTransactCache.cc
##########
@@ -1309,7 +1309,7 @@ HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
// If-Match: must match strongly //
if (request->presence(MIME_PRESENCE_IF_MATCH)) {
- int raw_etags_len, comma_sep_tag_list_len;
+ int raw_etags_len, comma_sep_tag_list_len = 0;
Review comment:
I think this is a different case, since the value is presumably set by value_get() in the next line.
Some advocate the practice of always initializing stack variables, to increase the repeatability of code behavior when debugging. We could consider defining and using a TS utility macro like:
```
#ifdef DEBUG
#define TS_DEFDZ(TYPE, NAME) TYPE NAME = 0
#else
#define TS_DEFDZ(TYPE, NAME) TYPE NAME
#endif
```
I wonder if gcc/clang has a compile option that will cause code to be generated to zero out stack allocations, that we could apply when configure --enable-debug was used.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall removed a comment on pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall removed a comment on pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#issuecomment-671671575
[approve ci docs]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zizhong commented on a change in pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
zizhong commented on a change in pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#discussion_r469569184
##########
File path: proxy/http/HttpTransactCache.cc
##########
@@ -1309,7 +1309,7 @@ HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
// If-Match: must match strongly //
if (request->presence(MIME_PRESENCE_IF_MATCH)) {
- int raw_etags_len, comma_sep_tag_list_len;
+ int raw_etags_len, comma_sep_tag_list_len = 0;
Review comment:
I agreed on what @maskit suggested.
1. Let's separate the declarations one per line.
2. and initialize each of them.
That way is much clearer than the current 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#issuecomment-673151641
[approve ci docs]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#discussion_r468661429
##########
File path: proxy/http/HttpBodyFactory.cc
##########
@@ -372,8 +372,8 @@ HttpBodyFactory::fabricate(StrList *acpt_language_list, StrList *acpt_charset_li
char *buffer;
const char *pType = context->txn_conf->body_factory_template_base;
const char *set;
- HttpBodyTemplate *t = nullptr;
- HttpBodySet *body_set;
+ HttpBodyTemplate *t = nullptr;
+ HttpBodySet *body_set = nullptr;
Review comment:
Also best moved to right before line 412.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#issuecomment-673095030
[approve ci clang-format]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#discussion_r468286689
##########
File path: proxy/http/HttpTransactCache.cc
##########
@@ -1309,7 +1309,7 @@ HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
// If-Match: must match strongly //
if (request->presence(MIME_PRESENCE_IF_MATCH)) {
- int raw_etags_len, comma_sep_tag_list_len;
+ int raw_etags_len, comma_sep_tag_list_len = 0;
Review comment:
I think `raw_etags_len` is still uninitialized. I usually don't define multiple variables on one line to avoid this confusion.
```c++
#include <iostream>
int main() {
int a, b = 1;
std::cout << a << std::endl;
std::cout << b << std::endl;
}
```
```
foo.cc:4:18: warning: variable 'a' is uninitialized when used here [-Wuninitialized]
std::cout << a << std::endl;
^
foo.cc:3:10: note: initialize the variable 'a' to silence this warning
int a, b = 1;
^
= 0
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#issuecomment-673097498
[approve ci docs]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall merged pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall merged pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#discussion_r468885181
##########
File path: proxy/http/HttpTransactCache.cc
##########
@@ -1309,7 +1309,7 @@ HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
// If-Match: must match strongly //
if (request->presence(MIME_PRESENCE_IF_MATCH)) {
- int raw_etags_len, comma_sep_tag_list_len;
+ int raw_etags_len, comma_sep_tag_list_len = 0;
Review comment:
> I think this is a different case, since the value is presumably set by value_get() in the next line.
Oh, I missed it. However, that proved this line is confusing. The reason I wrote the example code is I didn't remember the behavior and needed to check it. If I forgot the behavior, I might add a new variable on this line and forget initializing it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a change in pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall commented on a change in pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#discussion_r469399954
##########
File path: proxy/http/HttpTransactCache.cc
##########
@@ -1309,7 +1309,7 @@ HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
// If-Match: must match strongly //
if (request->presence(MIME_PRESENCE_IF_MATCH)) {
- int raw_etags_len, comma_sep_tag_list_len;
+ int raw_etags_len, comma_sep_tag_list_len = 0;
Review comment:
FTR, It was `comma_sep_tag_list_len` Xcode was warning on.
<img width="1269" alt="Screen Shot 2020-08-12 at 9 47 28 AM" src="https://user-images.githubusercontent.com/307125/90043371-decb2000-dc80-11ea-8ef8-90aa5be0ddd8.png">
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#discussion_r468660369
##########
File path: proxy/http/HttpBodyFactory.cc
##########
@@ -372,8 +372,8 @@ HttpBodyFactory::fabricate(StrList *acpt_language_list, StrList *acpt_charset_li
char *buffer;
const char *pType = context->txn_conf->body_factory_template_base;
const char *set;
- HttpBodyTemplate *t = nullptr;
- HttpBodySet *body_set;
+ HttpBodyTemplate *t = nullptr;
Review comment:
Seems better to also move this to right before line 412 to make code more readable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #7100: Fixes uninitialized variables found by Xcode
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #7100:
URL: https://github.com/apache/trafficserver/pull/7100#issuecomment-671671575
[approve ci docs]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org