SolidWallOfCode commented on a change in pull request #6520:
URL: https://github.com/apache/trafficserver/pull/6520#discussion_r568280595
##########
File path: proxy/http2/HPACK.cc
##########
@@ -946,22 +922,25 @@ hpack_encode_header_block(HpackIndexingTable
&indexing_table, uint8_t *out_buf,
MIMEFieldIter field_iter;
for (MIMEField *field = hdr->iter_get_first(&field_iter); field != nullptr;
field = hdr->iter_get_next(&field_iter)) {
Review comment:
That was actually my question - perhaps we should take `MIMEHdr` to do
that.
##########
File path: proxy/http2/HPACK.cc
##########
@@ -227,19 +223,15 @@ hpack_parse_field_type(uint8_t ftype)
namespace HpackStaticTable
{
HpackLookupResult
- lookup(const char *name, int name_len, const char *value, int value_len)
+ lookup(const HpackHeaderField &header)
{
HpackLookupResult result;
for (unsigned int index = 1; index < TS_HPACK_STATIC_TABLE_ENTRY_NUM;
++index) {
- const char *table_name = STATIC_TABLE[index].name;
- int table_name_len = STATIC_TABLE[index].name_size;
- const char *table_value = STATIC_TABLE[index].value;
- int table_value_len = STATIC_TABLE[index].value_size;
-
+ // TODO: replace `strcasecmp` with `compare`
// Check whether name (and value) are matched
- if (ptr_len_casecmp(name, name_len, table_name, table_name_len) == 0) {
- if ((value_len == table_value_len) && (memcmp(value, table_value,
value_len) == 0)) {
+ if (strcasecmp(header.name, STATIC_TABLE[index].name) == 0) {
+ if (header.value.compare(STATIC_TABLE[index].value) == 0) {
Review comment:
You need to compile with -O2 to see what happens in production code.
Checkout the results on godbolt.org - if you use this code
```
#include <string_view>
bool f(std::string_view const& lhs, std::string_view const& rhs) {
return lhs == rhs;
}
```
and compile with -O0 then you get the calls to compare. If you change that
to -O2, then with LLVM 11.0.1 you get this:
```
push rax
mov rdx, qword ptr [rdi]
cmp rdx, qword ptr [rsi] ; check lengths
jne .LBB0_5 ; return false if different.
test rdx, rdx ; same pointer?
je .LBB0_3 ; yep, return true.
mov rdi, qword ptr [rdi + 8]
mov rsi, qword ptr [rsi + 8]
call memcmp ; compare the strings.
```
##########
File path: proxy/http2/HPACK.cc
##########
@@ -227,19 +223,15 @@ hpack_parse_field_type(uint8_t ftype)
namespace HpackStaticTable
{
HpackLookupResult
- lookup(const char *name, int name_len, const char *value, int value_len)
+ lookup(const HpackHeaderField &header)
{
HpackLookupResult result;
for (unsigned int index = 1; index < TS_HPACK_STATIC_TABLE_ENTRY_NUM;
++index) {
- const char *table_name = STATIC_TABLE[index].name;
- int table_name_len = STATIC_TABLE[index].name_size;
- const char *table_value = STATIC_TABLE[index].value;
- int table_value_len = STATIC_TABLE[index].value_size;
-
+ // TODO: replace `strcasecmp` with `compare`
// Check whether name (and value) are matched
- if (ptr_len_casecmp(name, name_len, table_name, table_name_len) == 0) {
- if ((value_len == table_value_len) && (memcmp(value, table_value,
value_len) == 0)) {
+ if (strcasecmp(header.name, STATIC_TABLE[index].name) == 0) {
+ if (header.value.compare(STATIC_TABLE[index].value) == 0) {
Review comment:
You need to compile with -O2 to see what happens in production code.
Checkout the results on godbolt.org - if you use this code
```
#include <string_view>
bool f(std::string_view const& lhs, std::string_view const& rhs) {
return lhs == rhs;
}
```
and compile with -O0 then you get the calls to compare. If you change that
to -O2, then with LLVM 11.0.1 you get this:
```
push rax
mov rdx, qword ptr [rdi]
cmp rdx, qword ptr [rsi] ; check lengths
jne .LBB0_5 ; return false if different.
test rdx, rdx ; 0 length?
je .LBB0_3 ; yep, return true.
mov rdi, qword ptr [rdi + 8]
mov rsi, qword ptr [rsi + 8]
call memcmp ; compare the strings.
```
----------------------------------------------------------------
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:
[email protected]
