Improve HpackEntry class constructors.
Make constructor take std::string. This forces the call sites to explicitly
copy, making it clear that this class does not store reference (unlike
HpackLookupEntry).
Make it explicitly movable. This is trivial since the absl::string_view member
pointing to the std::string member has been removed at cr/363705447.
Make it non-copyable. This ensures that it is always moved.
Remove default constructor. It is never used, and it turns out that it is not
necessary for STL containers.
Making the type movable allows the use push() instead of emplace() cheaply in
accordance with guidance that Dianna brought to my attention at cl/363455100.
I was contemplating changing this class into a struct and removing the name()
and value() accessors, like HpackLookupEntry. However, that introduces a subtle
issue that I have already ran into in the past when I was exploring changing the
accessor return types from absl::string_view to const std::string&. When
inserting into static_name_index_ and dynamic_name_index_ maps, std::make_pair
is used as customary for map insertions:
http://google3/third_party/spdy/core/hpack/hpack_static_table.cc?l=41&rcl=364626329
http://google3/third_party/spdy/core/hpack/hpack_header_table.cc?l=169&rcl=364636382
However, if std::string name member is passed into make_pair, or the const
std::string& return value of an accessor, then make_pair makes a temporary
<std::string, size_t> pair by copying the string. This gets implicitly
converted into an <absl::string_view, size_t> pair by insert(), where the
string_view points to the temporary string which is destroyed right after the
string_view is inserted into the map, leaving a dangling reference. I could do
an explicit string_view conversion to avoid this, but instead I decided to keep
the less error-prone API.
PiperOrigin-RevId: 366834546
Change-Id: I00636f034010e74d8db0ab9a5ee3077148988349
diff --git a/spdy/core/hpack/hpack_entry.cc b/spdy/core/hpack/hpack_entry.cc
index 2e440f4..b8350df 100644
--- a/spdy/core/hpack/hpack_entry.cc
+++ b/spdy/core/hpack/hpack_entry.cc
@@ -10,8 +10,8 @@
namespace spdy {
-HpackEntry::HpackEntry(absl::string_view name, absl::string_view value)
- : name_(std::string(name)), value_(std::string(value)) {}
+HpackEntry::HpackEntry(std::string name, std::string value)
+ : name_(std::move(name)), value_(std::move(value)) {}
// static
size_t HpackEntry::Size(absl::string_view name, absl::string_view value) {
diff --git a/spdy/core/hpack/hpack_entry.h b/spdy/core/hpack/hpack_entry.h
index 281d225..0c26358 100644
--- a/spdy/core/hpack/hpack_entry.h
+++ b/spdy/core/hpack/hpack_entry.h
@@ -42,15 +42,26 @@
// and the header table (3.3.2).
class QUICHE_EXPORT_PRIVATE HpackEntry {
public:
- // Copies |name| and |value| in the constructor.
- HpackEntry(absl::string_view name, absl::string_view value);
+ HpackEntry(std::string name, std::string value);
- // Creates an entry with empty name and value. Only defined so that
- // entries can be stored in STL containers.
- HpackEntry() = default;
+ // Make HpackEntry non-copyable to make sure it is always moved.
+ HpackEntry(const HpackEntry&) = delete;
+ HpackEntry& operator=(const HpackEntry&) = delete;
- ~HpackEntry() = default;
+ HpackEntry(HpackEntry&&) = default;
+ HpackEntry& operator=(HpackEntry&&) = default;
+ // Getters for std::string members traditionally return const std::string&.
+ // However, HpackHeaderTable uses string_view as keys in the maps
+ // static_name_index_ and dynamic_name_index_. If HpackEntry::name() returned
+ // const std::string&, then
+ // dynamic_name_index_.insert(std::make_pair(entry.name(), index));
+ // would silently create a dangling reference: make_pair infers type from the
+ // return type of entry.name() and silently creates a temporary string copy.
+ // Insert creates a string_view that points to this copy, which then
+ // immediately goes out of scope and gets destroyed. While this is quite easy
+ // to avoid, for example, by explicitly specifying type as a template
+ // parameter to make_pair, returning string_view here is less error-prone.
absl::string_view name() const { return name_; }
absl::string_view value() const { return value_; }
diff --git a/spdy/core/hpack/hpack_entry_test.cc b/spdy/core/hpack/hpack_entry_test.cc
index 1a43c98..81acd9a 100644
--- a/spdy/core/hpack/hpack_entry_test.cc
+++ b/spdy/core/hpack/hpack_entry_test.cc
@@ -48,16 +48,6 @@
EXPECT_EQ(55u, HpackEntry::Size("header-name", "header value"));
}
-TEST(HpackEntryTest, DefaultConstructor) {
- HpackEntry entry;
-
- EXPECT_TRUE(entry.name().empty());
- EXPECT_TRUE(entry.value().empty());
-
- EXPECT_EQ(32u, entry.Size());
- EXPECT_EQ(32u, HpackEntry::Size("", ""));
-}
-
} // namespace
} // namespace spdy
diff --git a/spdy/core/hpack/hpack_header_table.cc b/spdy/core/hpack/hpack_header_table.cc
index 15c564e..ec69d47 100644
--- a/spdy/core/hpack/hpack_header_table.cc
+++ b/spdy/core/hpack/hpack_header_table.cc
@@ -134,7 +134,7 @@
const HpackEntry* HpackHeaderTable::TryAddEntry(absl::string_view name,
absl::string_view value) {
// Since |dynamic_entries_| has iterator stability, |name| and |value| are
- // valid even after evicting other entries and emplace_front() making room for
+ // valid even after evicting other entries and push_front() making room for
// the new one.
Evict(EvictionCountForEntry(name, value));
@@ -147,7 +147,8 @@
}
const size_t index = dynamic_table_insertions_;
- dynamic_entries_.emplace_front(name, value);
+ dynamic_entries_.push_front(
+ HpackEntry(std::string(name), std::string(value)));
HpackEntry* new_entry = &dynamic_entries_.front();
auto index_result = dynamic_index_.insert(std::make_pair(
HpackLookupEntry{new_entry->name(), new_entry->value()}, index));
diff --git a/spdy/core/hpack/hpack_static_table.cc b/spdy/core/hpack/hpack_static_table.cc
index 7eeb745..8c8248c 100644
--- a/spdy/core/hpack/hpack_static_table.cc
+++ b/spdy/core/hpack/hpack_static_table.cc
@@ -24,9 +24,9 @@
for (const HpackStaticEntry* it = static_entry_table;
it != static_entry_table + static_entry_count; ++it) {
- absl::string_view name(it->name, it->name_len);
- absl::string_view value(it->value, it->value_len);
- static_entries_.emplace_back(name, value);
+ std::string name(it->name, it->name_len);
+ std::string value(it->value, it->value_len);
+ static_entries_.push_back(HpackEntry(std::move(name), std::move(value)));
}
// |static_entries_| will not be mutated any more. Therefore its entries will
