Remove http2::HpackString class. Inlining std::string works just fine, no need for extra complexity. There are no plans to experiment with other string types. Change HpackStringPair and HpackDecoderTableEntry constructors to take std::string by value instead of const ref, and move in initializer list. This can be more efficient depending on how the constructor is called. Also remove unused HpackStringPair constructor. PiperOrigin-RevId: 362371728 Change-Id: Ifd4594be487632563cb81cb3703d90a1ed98bf1d
diff --git a/http2/hpack/decoder/hpack_decoder_listener.cc b/http2/hpack/decoder/hpack_decoder_listener.cc index a665be8..d1dea19 100644 --- a/http2/hpack/decoder/hpack_decoder_listener.cc +++ b/http2/hpack/decoder/hpack_decoder_listener.cc
@@ -13,8 +13,8 @@ HpackDecoderNoOpListener::~HpackDecoderNoOpListener() = default; void HpackDecoderNoOpListener::OnHeaderListStart() {} -void HpackDecoderNoOpListener::OnHeader(const HpackString& /*name*/, - const HpackString& /*value*/) {} +void HpackDecoderNoOpListener::OnHeader(const std::string& /*name*/, + const std::string& /*value*/) {} void HpackDecoderNoOpListener::OnHeaderListEnd() {} void HpackDecoderNoOpListener::OnHeaderErrorDetected( absl::string_view /*error_message*/) {}
diff --git a/http2/hpack/decoder/hpack_decoder_listener.h b/http2/hpack/decoder/hpack_decoder_listener.h index bbb0544..130e80b 100644 --- a/http2/hpack/decoder/hpack_decoder_listener.h +++ b/http2/hpack/decoder/hpack_decoder_listener.h
@@ -28,7 +28,7 @@ // Called for each header name-value pair that is decoded, in the order they // appear in the HPACK block. Multiple values for a given key will be emitted // as multiple calls to OnHeader. - virtual void OnHeader(const HpackString& name, const HpackString& value) = 0; + virtual void OnHeader(const std::string& name, const std::string& value) = 0; // OnHeaderListEnd is called after successfully decoding an HPACK block into // an HTTP/2 header list. Will only be called once per block, even if it @@ -49,7 +49,7 @@ ~HpackDecoderNoOpListener() override; void OnHeaderListStart() override; - void OnHeader(const HpackString& name, const HpackString& value) override; + void OnHeader(const std::string& name, const std::string& value) override; void OnHeaderListEnd() override; void OnHeaderErrorDetected(absl::string_view error_message) override;
diff --git a/http2/hpack/decoder/hpack_decoder_state.cc b/http2/hpack/decoder/hpack_decoder_state.cc index 528f1b6..86df71b 100644 --- a/http2/hpack/decoder/hpack_decoder_state.cc +++ b/http2/hpack/decoder/hpack_decoder_state.cc
@@ -12,11 +12,11 @@ namespace http2 { namespace { -HpackString ExtractHpackString(HpackDecoderStringBuffer* string_buffer) { +std::string ExtractString(HpackDecoderStringBuffer* string_buffer) { if (string_buffer->IsBuffered()) { - return HpackString(string_buffer->ReleaseString()); + return string_buffer->ReleaseString(); } else { - auto result = HpackString(string_buffer->str()); + auto result = std::string(string_buffer->str()); string_buffer->Reset(); return result; } @@ -114,7 +114,7 @@ allow_dynamic_table_size_update_ = false; const HpackStringPair* entry = decoder_tables_.Lookup(name_index); if (entry != nullptr) { - HpackString value(ExtractHpackString(value_buffer)); + std::string value(ExtractString(value_buffer)); listener_->OnHeader(entry->name, value); if (entry_type == HpackEntryType::kIndexedLiteralHeader) { decoder_tables_.Insert(entry->name, value); @@ -138,8 +138,8 @@ return; } allow_dynamic_table_size_update_ = false; - HpackString name(ExtractHpackString(name_buffer)); - HpackString value(ExtractHpackString(value_buffer)); + std::string name(ExtractString(name_buffer)); + std::string value(ExtractString(value_buffer)); listener_->OnHeader(name, value); if (entry_type == HpackEntryType::kIndexedLiteralHeader) { decoder_tables_.Insert(name, value);
diff --git a/http2/hpack/decoder/hpack_decoder_state_test.cc b/http2/hpack/decoder/hpack_decoder_state_test.cc index 5001565..8dca4cd 100644 --- a/http2/hpack/decoder/hpack_decoder_state_test.cc +++ b/http2/hpack/decoder/hpack_decoder_state_test.cc
@@ -38,7 +38,7 @@ public: MOCK_METHOD0(OnHeaderListStart, void()); MOCK_METHOD2(OnHeader, - void(const HpackString& name, const HpackString& value)); + void(const std::string& name, const std::string& value)); MOCK_METHOD0(OnHeaderListEnd, void()); MOCK_METHOD(void, OnHeaderErrorDetected, @@ -155,8 +155,8 @@ const HpackStringPair* entry = Lookup(dynamic_index + kFirstDynamicTableIndex - 1); VERIFY_NE(entry, nullptr); - VERIFY_EQ(entry->name.ToStringPiece(), name); - VERIFY_EQ(entry->value.ToStringPiece(), value); + VERIFY_EQ(entry->name, name); + VERIFY_EQ(entry->value, value); return AssertionSuccess(); } AssertionResult VerifyNoEntry(size_t dynamic_index) {
diff --git a/http2/hpack/decoder/hpack_decoder_tables.cc b/http2/hpack/decoder/hpack_decoder_tables.cc index 1b2cc11..203aae0 100644 --- a/http2/hpack/decoder/hpack_decoder_tables.cc +++ b/http2/hpack/decoder/hpack_decoder_tables.cc
@@ -51,9 +51,9 @@ } HpackDecoderDynamicTable::HpackDecoderTableEntry::HpackDecoderTableEntry( - const HpackString& name, - const HpackString& value) - : HpackStringPair(name, value) {} + std::string name_arg, + std::string value_arg) + : HpackStringPair(std::move(name_arg), std::move(value_arg)) {} HpackDecoderDynamicTable::HpackDecoderDynamicTable() : insert_count_(kFirstDynamicTableIndex - 1), debug_listener_(nullptr) {} @@ -69,8 +69,8 @@ // TODO(jamessynge): Check somewhere before here that names received from the // peer are valid (e.g. are lower-case, no whitespace, etc.). -void HpackDecoderDynamicTable::Insert(const HpackString& name, - const HpackString& value) { +void HpackDecoderDynamicTable::Insert(const std::string& name, + const std::string& value) { HpackDecoderTableEntry entry(name, value); size_t entry_size = entry.size(); HTTP2_DVLOG(2) << "InsertEntry of size=" << entry_size
diff --git a/http2/hpack/decoder/hpack_decoder_tables.h b/http2/hpack/decoder/hpack_decoder_tables.h index 5f56cf2..6ad8ca9 100644 --- a/http2/hpack/decoder/hpack_decoder_tables.h +++ b/http2/hpack/decoder/hpack_decoder_tables.h
@@ -108,7 +108,7 @@ // Insert entry if possible. // If entry is too large to insert, then dynamic table will be empty. - void Insert(const HpackString& name, const HpackString& value); + void Insert(const std::string& name, const std::string& value); // If index is valid, returns a pointer to the entry, otherwise returns // nullptr. @@ -120,7 +120,7 @@ private: friend class test::HpackDecoderTablesPeer; struct HpackDecoderTableEntry : public HpackStringPair { - HpackDecoderTableEntry(const HpackString& name, const HpackString& value); + HpackDecoderTableEntry(std::string name_arg, std::string value_arg); int64_t time_added; }; @@ -169,7 +169,7 @@ // If entry is too large to insert, then dynamic table will be empty. // TODO(jamessynge): Add methods for moving the string(s) into the table, // or for otherwise avoiding unnecessary copies. - void Insert(const HpackString& name, const HpackString& value) { + void Insert(const std::string& name, const std::string& value) { dynamic_table_.Insert(name, value); }
diff --git a/http2/hpack/decoder/hpack_decoder_tables_test.cc b/http2/hpack/decoder/hpack_decoder_tables_test.cc index a3e4ee0..b7ed527 100644 --- a/http2/hpack/decoder/hpack_decoder_tables_test.cc +++ b/http2/hpack/decoder/hpack_decoder_tables_test.cc
@@ -205,7 +205,7 @@ // move up by 1 index. AssertionResult Insert(const std::string& name, const std::string& value) { size_t old_count = num_dynamic_entries(); - tables_.Insert(HpackString(name), HpackString(value)); + tables_.Insert(name, value); FakeInsert(name, value); VERIFY_EQ(old_count + 1, fake_dynamic_table_.size()); FakeTrim(dynamic_size_limit());
diff --git a/http2/hpack/decoder/hpack_decoder_test.cc b/http2/hpack/decoder/hpack_decoder_test.cc index dc6ab58..11a1987 100644 --- a/http2/hpack/decoder/hpack_decoder_test.cc +++ b/http2/hpack/decoder/hpack_decoder_test.cc
@@ -65,7 +65,7 @@ public: MOCK_METHOD0(OnHeaderListStart, void()); MOCK_METHOD2(OnHeader, - void(const HpackString& name, const HpackString& value)); + void(const std::string& name, const std::string& value)); MOCK_METHOD0(OnHeaderListEnd, void()); MOCK_METHOD(void, OnHeaderErrorDetected, @@ -95,10 +95,10 @@ // Called for each header name-value pair that is decoded, in the order they // appear in the HPACK block. Multiple values for a given key will be emitted // as multiple calls to OnHeader. - void OnHeader(const HpackString& name, const HpackString& value) override { + void OnHeader(const std::string& name, const std::string& value) override { ASSERT_TRUE(saw_start_); ASSERT_FALSE(saw_end_); - header_entries_.emplace_back(name.ToString(), value.ToString()); + header_entries_.emplace_back(name, value); } // OnHeaderBlockEnd is called after successfully decoding an HPACK block. Will @@ -190,8 +190,8 @@ const HpackStringPair* entry = Lookup(dynamic_index + kFirstDynamicTableIndex - 1); VERIFY_NE(entry, nullptr); - VERIFY_EQ(entry->name.ToStringPiece(), name); - VERIFY_EQ(entry->value.ToStringPiece(), value); + VERIFY_EQ(entry->name, name); + VERIFY_EQ(entry->value, value); return AssertionSuccess(); } AssertionResult VerifyNoEntry(size_t dynamic_index) {
diff --git a/http2/hpack/hpack_string.cc b/http2/hpack/hpack_string.cc index fb9691c..ef7af73 100644 --- a/http2/hpack/hpack_string.cc +++ b/http2/hpack/hpack_string.cc
@@ -11,48 +11,8 @@ namespace http2 { -HpackString::HpackString(const char* data) : str_(data) {} -HpackString::HpackString(absl::string_view str) : str_(std::string(str)) {} -HpackString::HpackString(std::string str) : str_(std::move(str)) {} -HpackString::HpackString(const HpackString& other) = default; -HpackString::~HpackString() = default; - -absl::string_view HpackString::ToStringPiece() const { - return str_; -} - -bool HpackString::operator==(const HpackString& other) const { - return str_ == other.str_; -} -bool HpackString::operator==(absl::string_view str) const { - return str == str_; -} - -bool operator==(absl::string_view a, const HpackString& b) { - return b == a; -} -bool operator!=(absl::string_view a, const HpackString& b) { - return !(b == a); -} -bool operator!=(const HpackString& a, const HpackString& b) { - return !(a == b); -} -bool operator!=(const HpackString& a, absl::string_view b) { - return !(a == b); -} -std::ostream& operator<<(std::ostream& out, const HpackString& v) { - return out << v.ToString(); -} - -HpackStringPair::HpackStringPair(const HpackString& name, - const HpackString& value) - : name(name), value(value) { - HTTP2_DVLOG(3) << DebugString() << " ctor"; -} - -HpackStringPair::HpackStringPair(absl::string_view name, - absl::string_view value) - : name(name), value(value) { +HpackStringPair::HpackStringPair(std::string name, std::string value) + : name(std::move(name)), value(std::move(value)) { HTTP2_DVLOG(3) << DebugString() << " ctor"; } @@ -61,8 +21,7 @@ } std::string HpackStringPair::DebugString() const { - return absl::StrCat("HpackStringPair(name=", name.ToString(), - ", value=", value.ToString(), ")"); + return absl::StrCat("HpackStringPair(name=", name, ", value=", value, ")"); } std::ostream& operator<<(std::ostream& os, const HpackStringPair& p) {
diff --git a/http2/hpack/hpack_string.h b/http2/hpack/hpack_string.h index 7896046..51520e8 100644 --- a/http2/hpack/hpack_string.h +++ b/http2/hpack/hpack_string.h
@@ -5,11 +5,6 @@ #ifndef QUICHE_HTTP2_HPACK_HPACK_STRING_H_ #define QUICHE_HTTP2_HPACK_HPACK_STRING_H_ -// HpackString is currently a very simple container for a string, but allows us -// to relatively easily experiment with alternate string storage mechanisms for -// handling strings to be encoded with HPACK, or decoded from HPACK, such as -// a ref-counted string. - #include <stddef.h> #include <iosfwd> @@ -20,44 +15,8 @@ namespace http2 { -class QUICHE_EXPORT_PRIVATE HpackString { - public: - explicit HpackString(const char* data); - explicit HpackString(absl::string_view str); - explicit HpackString(std::string str); - HpackString(const HpackString& other); - - // Not sure yet whether this move ctor is required/sensible. - HpackString(HpackString&& other) = default; - - ~HpackString(); - - size_t size() const { return str_.size(); } - const std::string& ToString() const { return str_; } - absl::string_view ToStringPiece() const; - - bool operator==(const HpackString& other) const; - - bool operator==(absl::string_view str) const; - - private: - std::string str_; -}; - -QUICHE_EXPORT_PRIVATE bool operator==(absl::string_view a, - const HpackString& b); -QUICHE_EXPORT_PRIVATE bool operator!=(absl::string_view a, - const HpackString& b); -QUICHE_EXPORT_PRIVATE bool operator!=(const HpackString& a, - const HpackString& b); -QUICHE_EXPORT_PRIVATE bool operator!=(const HpackString& a, - absl::string_view b); -QUICHE_EXPORT_PRIVATE std::ostream& operator<<(std::ostream& out, - const HpackString& v); - struct QUICHE_EXPORT_PRIVATE HpackStringPair { - HpackStringPair(const HpackString& name, const HpackString& value); - HpackStringPair(absl::string_view name, absl::string_view value); + HpackStringPair(std::string name, std::string value); ~HpackStringPair(); // Returns the size of a header entry with this name and value, per the RFC: @@ -66,8 +25,8 @@ std::string DebugString() const; - const HpackString name; - const HpackString value; + const std::string name; + const std::string value; }; QUICHE_EXPORT_PRIVATE std::ostream& operator<<(std::ostream& os,
diff --git a/http2/hpack/hpack_string_test.cc b/http2/hpack/hpack_string_test.cc deleted file mode 100644 index c8462e8..0000000 --- a/http2/hpack/hpack_string_test.cc +++ /dev/null
@@ -1,149 +0,0 @@ -// Copyright 2016 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "http2/hpack/hpack_string.h" - -// Tests of HpackString. - -#include <utility> - -#include "http2/platform/api/http2_logging.h" -#include "http2/platform/api/http2_test_helpers.h" -#include "common/platform/api/quiche_test.h" - -using ::testing::AssertionFailure; -using ::testing::AssertionResult; -using ::testing::AssertionSuccess; - -namespace http2 { -namespace test { -namespace { - -const char kStr0[] = "s0: Some string to be copied into another string."; -const char kStr1[] = "S1 - some string to be copied into yet another string."; - -class HpackStringTest : public QuicheTest { - protected: - AssertionResult VerifyNotEqual(HpackString* actual, - const std::string& not_expected_str) { - const char* not_expected_ptr = not_expected_str.c_str(); - absl::string_view not_expected_sp(not_expected_str); - - VERIFY_NE(*actual, not_expected_ptr); - VERIFY_NE(*actual, not_expected_sp); - VERIFY_NE(*actual, not_expected_str); - VERIFY_NE(actual->ToStringPiece(), not_expected_sp); - - if (!(not_expected_ptr != *actual)) { - return AssertionFailure(); - } - if (!(not_expected_sp != *actual)) { - return AssertionFailure(); - } - if (!(not_expected_str != *actual)) { - return AssertionFailure(); - } - if (!(not_expected_sp != actual->ToStringPiece())) { - return AssertionFailure(); - } - - return AssertionSuccess(); - } - - AssertionResult VerifyEqual(HpackString* actual, - const std::string& expected_str) { - VERIFY_EQ(actual->size(), expected_str.size()); - - const char* expected_ptr = expected_str.c_str(); - const absl::string_view expected_sp(expected_str); - - VERIFY_EQ(*actual, expected_ptr); - VERIFY_EQ(*actual, expected_sp); - VERIFY_EQ(*actual, expected_str); - VERIFY_EQ(actual->ToStringPiece(), expected_sp); - - if (!(expected_sp == *actual)) { - return AssertionFailure(); - } - if (!(expected_ptr == *actual)) { - return AssertionFailure(); - } - if (!(expected_str == *actual)) { - return AssertionFailure(); - } - if (!(expected_sp == actual->ToStringPiece())) { - return AssertionFailure(); - } - - return AssertionSuccess(); - } -}; - -TEST_F(HpackStringTest, CharArrayConstructor) { - HpackString hs0(kStr0); - EXPECT_TRUE(VerifyEqual(&hs0, kStr0)); - EXPECT_TRUE(VerifyNotEqual(&hs0, kStr1)); - - HpackString hs1(kStr1); - EXPECT_TRUE(VerifyEqual(&hs1, kStr1)); - EXPECT_TRUE(VerifyNotEqual(&hs1, kStr0)); -} - -TEST_F(HpackStringTest, StringPieceConstructor) { - absl::string_view sp0(kStr0); - HpackString hs0(sp0); - EXPECT_TRUE(VerifyEqual(&hs0, kStr0)); - EXPECT_TRUE(VerifyNotEqual(&hs0, kStr1)); - - absl::string_view sp1(kStr1); - HpackString hs1(sp1); - EXPECT_TRUE(VerifyEqual(&hs1, kStr1)); - EXPECT_TRUE(VerifyNotEqual(&hs1, kStr0)); -} - -TEST_F(HpackStringTest, MoveStringConstructor) { - std::string str0(kStr0); - HpackString hs0(str0); - EXPECT_TRUE(VerifyEqual(&hs0, kStr0)); - EXPECT_TRUE(VerifyNotEqual(&hs0, kStr1)); - - std::string str1(kStr1); - HpackString hs1(str1); - EXPECT_TRUE(VerifyEqual(&hs1, kStr1)); - EXPECT_TRUE(VerifyNotEqual(&hs1, kStr0)); -} - -TEST_F(HpackStringTest, CopyConstructor) { - absl::string_view sp0(kStr0); - HpackString hs0(sp0); - HpackString hs1(hs0); - EXPECT_EQ(hs0, hs1); - - EXPECT_TRUE(VerifyEqual(&hs0, kStr0)); - EXPECT_TRUE(VerifyEqual(&hs1, kStr0)); - - EXPECT_TRUE(VerifyNotEqual(&hs0, kStr1)); - EXPECT_TRUE(VerifyNotEqual(&hs1, kStr1)); -} - -TEST_F(HpackStringTest, MoveConstructor) { - absl::string_view sp0(kStr0); - HpackString hs0(sp0); - EXPECT_TRUE(VerifyEqual(&hs0, kStr0)); - EXPECT_TRUE(VerifyNotEqual(&hs0, "")); - - HpackString hs1(std::move(hs0)); - EXPECT_NE(hs0, hs1); - - EXPECT_TRUE(VerifyEqual(&hs1, kStr0)); - EXPECT_TRUE(VerifyEqual(&hs0, "")); - EXPECT_TRUE(VerifyNotEqual(&hs1, "")); - - HTTP2_LOG(INFO) << hs0; - HTTP2_LOG(INFO) << hs1; -} - -} // namespace -} // namespace test -} // namespace http2
diff --git a/spdy/core/hpack/hpack_decoder_adapter.cc b/spdy/core/hpack/hpack_decoder_adapter.cc index 59ba669..6b9b425 100644 --- a/spdy/core/hpack/hpack_decoder_adapter.cc +++ b/spdy/core/hpack/hpack_decoder_adapter.cc
@@ -11,7 +11,6 @@ #include "spdy/platform/api/spdy_logging.h" using ::http2::DecodeBuffer; -using ::http2::HpackString; namespace spdy { namespace { @@ -159,18 +158,17 @@ } } -void HpackDecoderAdapter::ListenerAdapter::OnHeader(const HpackString& name, - const HpackString& value) { +void HpackDecoderAdapter::ListenerAdapter::OnHeader(const std::string& name, + const std::string& value) { SPDY_DVLOG(2) << "HpackDecoderAdapter::ListenerAdapter::OnHeader:\n name: " << name << "\n value: " << value; total_uncompressed_bytes_ += name.size() + value.size(); if (handler_ == nullptr) { SPDY_DVLOG(3) << "Adding to decoded_block"; - decoded_block_.AppendValueOrAddHeader(name.ToStringPiece(), - value.ToStringPiece()); + decoded_block_.AppendValueOrAddHeader(name, value); } else { SPDY_DVLOG(3) << "Passing to handler"; - handler_->OnHeader(name.ToStringPiece(), value.ToStringPiece()); + handler_->OnHeader(name, value); } } @@ -197,8 +195,7 @@ if (visitor_ == nullptr) { return 0; } - HpackEntry hpack_entry(entry.name.ToStringPiece(), - entry.value.ToStringPiece(), + HpackEntry hpack_entry(entry.name, entry.value, /*is_static*/ false, insert_count); int64_t time_added = visitor_->OnNewEntry(hpack_entry); SPDY_DVLOG(2) @@ -215,8 +212,7 @@ << ", insert_count=" << insert_count << ", time_added=" << time_added; if (visitor_ != nullptr) { - HpackEntry hpack_entry(entry.name.ToStringPiece(), - entry.value.ToStringPiece(), /*is_static*/ false, + HpackEntry hpack_entry(entry.name, entry.value, /*is_static*/ false, insert_count); hpack_entry.set_time_added(time_added); visitor_->OnUseEntry(hpack_entry);
diff --git a/spdy/core/hpack/hpack_decoder_adapter.h b/spdy/core/hpack/hpack_decoder_adapter.h index 5ec7046..2a21fd0 100644 --- a/spdy/core/hpack/hpack_decoder_adapter.h +++ b/spdy/core/hpack/hpack_decoder_adapter.h
@@ -110,8 +110,7 @@ // Override the HpackDecoderListener methods: void OnHeaderListStart() override; - void OnHeader(const http2::HpackString& name, - const http2::HpackString& value) override; + void OnHeader(const std::string& name, const std::string& value) override; void OnHeaderListEnd() override; void OnHeaderErrorDetected(absl::string_view error_message) override;
diff --git a/spdy/core/hpack/hpack_decoder_adapter_test.cc b/spdy/core/hpack/hpack_decoder_adapter_test.cc index c768755..0a12fe4 100644 --- a/spdy/core/hpack/hpack_decoder_adapter_test.cc +++ b/spdy/core/hpack/hpack_decoder_adapter_test.cc
@@ -28,7 +28,6 @@ #include "spdy/platform/api/spdy_string_utils.h" using ::http2::HpackEntryType; -using ::http2::HpackString; using ::http2::HpackStringPair; using ::http2::test::HpackBlockBuilder; using ::http2::test::HpackDecoderPeer; @@ -66,9 +65,9 @@ explicit HpackDecoderAdapterPeer(HpackDecoderAdapter* decoder) : decoder_(decoder) {} - void HandleHeaderRepresentation(absl::string_view name, - absl::string_view value) { - decoder_->listener_adapter_.OnHeader(HpackString(name), HpackString(value)); + void HandleHeaderRepresentation(const std::string& name, + const std::string& value) { + decoder_->listener_adapter_.OnHeader(name, value); } http2::HpackDecoderTables* GetDecoderTables() {