Replace ParameterizedMember implementation with std::variant By introducing a new InnerList struct. This facilitates the introduction of GetIfItem and GetIfInnerList methods that directly return a pointer to either the ParameterizedItem or the InnerList, respectively, and makes it so that the ParameterizedMember class's invariants are automatically upheld, as compared to the previously fragile member/member_is_inner_list/params fields. All users of those fields have been migrated to the GetWithParamsIf* methods, which are retained but deprecated; callers will be migrated to GetIfItem and GetIfInnerList, after which the GetWithParamsIf* methods will be removed. The new Inner list type is a struct, rather than a class, as it is effectively a trivial analogue of std::pair with nicer field names. This contains no intentional observable behavioral differences. PiperOrigin-RevId: 978493551
diff --git a/quiche/common/structured_headers.cc b/quiche/common/structured_headers.cc index b0ca288..78b7dfe 100644 --- a/quiche/common/structured_headers.cc +++ b/quiche/common/structured_headers.cc
@@ -710,9 +710,9 @@ if (!first) output_ << ", "; if (!WriteKey(dict_key)) return false; first = false; - if (!dict_value.member_is_inner_list && !dict_value.member.empty() && - IsBooleanTrue(dict_value.member.front().item)) { - if (!WriteParameters(dict_value.params)) return false; + if (const auto* item = dict_value.GetIfItem(); + item && IsBooleanTrue(item->item)) { + if (!WriteParameters(item->params)) return false; } else { output_ << "="; if (!WriteParameterizedMember(dict_value)) return false; @@ -730,27 +730,29 @@ [[nodiscard]] bool WriteParameterizedMember( const ParameterizedMember& value) { // Serializes a parameterized member ([RFC8941] 4.1.1). - if (value.member_is_inner_list) { - if (!WriteInnerList(value.member)) return false; - } else { - QUICHE_CHECK_EQ(value.member.size(), 1UL); - if (!WriteItem(value.member[0])) return false; - } - return WriteParameters(value.params); + return std::visit( + absl::Overload{ + [&](const ParameterizedItem& value) { return WriteItem(value); }, + [&](const InnerList& value) { return WriteInnerList(value); }, + [](std::monostate) { + QUICHE_CHECK(false); + return false; + }, + }, + value.value_); } - [[nodiscard]] bool WriteInnerList( - const std::vector<ParameterizedItem>& value) { + [[nodiscard]] bool WriteInnerList(const InnerList& value) { // Serializes an inner list ([RFC8941] 4.1.1.1). output_ << "("; bool first = true; - for (const ParameterizedItem& member : value) { + for (const ParameterizedItem& member : value.items) { if (!first) output_ << " "; if (!WriteItem(member)) return false; first = false; } output_ << ")"; - return true; + return WriteParameters(value.params); } [[nodiscard]] bool WriteParameters(const Parameters& value) { @@ -899,6 +901,24 @@ ParameterizedItem::ParameterizedItem(Item item) : item(std::move(item)) {} ParameterizedItem::~ParameterizedItem() = default; +InnerList::InnerList() = default; + +InnerList::InnerList(std::vector<ParameterizedItem> items) + : items(std::move(items)) {} + +InnerList::InnerList(std::vector<ParameterizedItem> items, Parameters params) + : items(std::move(items)), params(std::move(params)) {} + +InnerList::InnerList(const InnerList&) = default; + +InnerList& InnerList::operator=(const InnerList&) = default; + +InnerList::InnerList(InnerList&&) = default; + +InnerList& InnerList::operator=(InnerList&&) = default; + +InnerList::~InnerList() = default; + ParameterizedMember::ParameterizedMember() = default; ParameterizedMember::ParameterizedMember(const ParameterizedMember&) = default; ParameterizedMember& ParameterizedMember::operator=( @@ -906,70 +926,94 @@ ParameterizedMember::ParameterizedMember(ParameterizedMember&&) = default; ParameterizedMember& ParameterizedMember::operator=(ParameterizedMember&&) = default; -ParameterizedMember::ParameterizedMember(std::vector<ParameterizedItem> items, - bool member_is_inner_list, - Parameters params) - : member(std::move(items)), - member_is_inner_list(member_is_inner_list), - params(std::move(params)) {} + ParameterizedMember::ParameterizedMember(std::vector<ParameterizedItem> items, Parameters params) - : member(std::move(items)), - member_is_inner_list(true), - params(std::move(params)) {} + : value_(std::in_place_type<InnerList>, std::move(items), + std::move(params)) {} + ParameterizedMember::ParameterizedMember(std::vector<ParameterizedItem> items) - : member(std::move(items)), member_is_inner_list(true) {} + : value_(std::in_place_type<InnerList>, std::move(items)) {} + ParameterizedMember::ParameterizedMember(Item item, Parameters params) - : member({{std::move(item), {}}}), - member_is_inner_list(false), - params(std::move(params)) {} + : value_(std::in_place_type<ParameterizedItem>, std::move(item), + std::move(params)) {} + ParameterizedMember::ParameterizedMember(Item item) - : member({{std::move(item), {}}}), member_is_inner_list(false) {} + : value_(std::in_place_type<ParameterizedItem>, std::move(item)) {} + +ParameterizedMember::ParameterizedMember(ParameterizedItem item) + : value_(std::move(item)) {} + +ParameterizedMember::ParameterizedMember(InnerList inner_list) + : value_(std::move(inner_list)) {} + ParameterizedMember::~ParameterizedMember() = default; +const ParameterizedItem* ParameterizedMember::GetIfItem() const { + return std::get_if<ParameterizedItem>(&value_); +} + +ParameterizedItem* ParameterizedMember::GetIfItem() { + return std::get_if<ParameterizedItem>(&value_); +} + +const InnerList* ParameterizedMember::GetIfInnerList() const { + return std::get_if<InnerList>(&value_); +} + +InnerList* ParameterizedMember::GetIfInnerList() { + return std::get_if<InnerList>(&value_); +} + std::optional<std::pair<const Item&, const Parameters&>> ParameterizedMember::GetWithParamsIfItem() const { - // Strictly, `member.size()` should be exactly 1 when `!member_is_inner_list`, - // but this isn't guaranteed due to to the public nature of the fields. Handle - // the empty case here to avoid crashing or UB. - if (member_is_inner_list || member.empty()) { + const auto* item = GetIfItem(); + if (!item) { return std::nullopt; } - return std::pair<const Item&, const Parameters&>(member.front().item, params); + return std::pair<const Item&, const Parameters&>(item->item, item->params); } std::optional<std::pair<Item&, Parameters&>> ParameterizedMember::GetWithParamsIfItem() { - // Strictly, `member.size()` should be exactly 1 when `!member_is_inner_list`, - // but this isn't guaranteed due to to the public nature of the fields. Handle - // the empty case here to avoid crashing or UB. - if (member_is_inner_list || member.empty()) { + auto* item = GetIfItem(); + if (!item) { return std::nullopt; } - return std::pair<Item&, Parameters&>(member.front().item, params); + return std::pair<Item&, Parameters&>(item->item, item->params); } std::optional< std::pair<const std::vector<ParameterizedItem>&, const Parameters&>> ParameterizedMember::GetWithParamsIfInnerList() const { - if (!member_is_inner_list) { + const auto* inner_list = GetIfInnerList(); + if (!inner_list) { return std::nullopt; } return std::pair<const std::vector<ParameterizedItem>&, const Parameters&>( - member, params); + inner_list->items, inner_list->params); } std::optional<std::pair<std::vector<ParameterizedItem>&, Parameters&>> ParameterizedMember::GetWithParamsIfInnerList() { - if (!member_is_inner_list) { + auto* inner_list = GetIfInnerList(); + if (!inner_list) { return std::nullopt; } - return std::pair<std::vector<ParameterizedItem>&, Parameters&>(member, - params); + return std::pair<std::vector<ParameterizedItem>&, Parameters&>( + inner_list->items, inner_list->params); +} + +// Not defaulted to work around +// https://github.com/llvm/llvm-project/issues/132249 in older Clang versions. +bool operator==(const ParameterizedMember& lhs, + const ParameterizedMember& rhs) { + return lhs.value_ == rhs.value_; } ParameterisedIdentifier::ParameterisedIdentifier() = default;
diff --git a/quiche/common/structured_headers.h b/quiche/common/structured_headers.h index 0ee810a..efb9a2a 100644 --- a/quiche/common/structured_headers.h +++ b/quiche/common/structured_headers.h
@@ -221,13 +221,36 @@ const ParameterizedItem&) = default; }; -// Holds a ParameterizedMember, which may be either a single Item, or an Inner -// List of ParameterizedItems, along with any number of parameters. Parameter -// ordering is significant. +// https://www.rfc-editor.org/rfc/rfc8941.html#name-inner-lists +struct QUICHE_EXPORT InnerList { + std::vector<ParameterizedItem> items; + Parameters params; + + InnerList(); + + explicit InnerList(std::vector<ParameterizedItem> items); + + InnerList(std::vector<ParameterizedItem> items, Parameters params); + + InnerList(const InnerList&); + InnerList& operator=(const InnerList&); + + InnerList(InnerList&&); + InnerList& operator=(InnerList&&); + + ~InnerList(); + + friend bool operator==(const InnerList&, const InnerList&) = default; +}; + +// Holds either a `ParameterizedItem` or an `InnerList`. // -// TODO(b/517204961): Replace the `member`, `member_is_inner_list`, and `params` -// fields with `std::variant<ParameterizedItem, InnerList>`. +// TODO(apaseltiner): Use `class` instead of `struct`, since some members are +// private. struct QUICHE_EXPORT ParameterizedMember { + explicit ParameterizedMember(ParameterizedItem); + explicit ParameterizedMember(InnerList); + // Constructor for a member that is an inner list. ParameterizedMember(std::vector<ParameterizedItem>, Parameters); @@ -250,55 +273,58 @@ ~ParameterizedMember(); + const ParameterizedItem* GetIfItem() const ABSL_ATTRIBUTE_LIFETIME_BOUND; + ParameterizedItem* GetIfItem() ABSL_ATTRIBUTE_LIFETIME_BOUND; + + const InnerList* GetIfInnerList() const ABSL_ATTRIBUTE_LIFETIME_BOUND; + InnerList* GetIfInnerList() ABSL_ATTRIBUTE_LIFETIME_BOUND; + // Returns the item and its parameters if the member is an item, // `std::nullopt` otherwise. + // + // Deprecated: Use `GetIfItem()` instead. std::optional<std::pair<const Item&, const Parameters&>> GetWithParamsIfItem() const ABSL_ATTRIBUTE_LIFETIME_BOUND; // Returns the item and its parameters if the member is an item, // `std::nullopt` otherwise. + // + // Deprecated: Use `GetIfItem()` instead. std::optional<std::pair<Item&, Parameters&>> GetWithParamsIfItem() ABSL_ATTRIBUTE_LIFETIME_BOUND; // Returns the inner list's items and its parameters if the member is an // inner list, `std::nullopt` otherwise. + // + // Deprecated: Use `GetIfInnerList()` instead. std::optional< std::pair<const std::vector<ParameterizedItem>&, const Parameters&>> GetWithParamsIfInnerList() const ABSL_ATTRIBUTE_LIFETIME_BOUND; // Returns the inner list's items and its parameters if the member is an // inner list, `std::nullopt` otherwise. + // + // Deprecated: Use `GetIfInnerList()` instead. std::optional<std::pair<std::vector<ParameterizedItem>&, Parameters&>> GetWithParamsIfInnerList() ABSL_ATTRIBUTE_LIFETIME_BOUND; - friend bool operator==(const ParameterizedMember&, - const ParameterizedMember&) = default; + QUICHE_EXPORT friend bool operator==(const ParameterizedMember&, + const ParameterizedMember&); // Deprecated: Explicitly initialize the value to either an inner list or // an item using one of the above constructors, or wrap the value in // `std::optional`. This constructor shouldn't really exist, as it's not clear // what the default should actually be, but it is convenient for code that - // defers assignment. As is, it produces an invalid value with - // `member.empty() && !member_is_inner_list`. + // defers assignment. As is, it produces an invalid value where both + // `GetIfItem()` and `GetIfInnerList()` return `nullptr`. ParameterizedMember(); - // Deprecated: Use either of the two-argument constructors depending on - // whether the value is an inner list or an item. - ParameterizedMember(std::vector<ParameterizedItem>, bool member_is_inner_list, - Parameters); + private: + friend class StructuredHeaderSerializer; - // Deprecated: Use `GetWithParamsIfItem()` / `GetWithParamsIfInnerList()` - // instead. - std::vector<ParameterizedItem> member; - - // If false, then |member| should only hold one Item. - // Deprecated: Use `GetWithParamsIfItem()` / `GetWithParamsIfInnerList()` - // instead. - bool member_is_inner_list = false; - - // Deprecated: Use `GetWithParamsIfItem()` / `GetWithParamsIfInnerList()` - // instead. - Parameters params; + // TODO(apaseltiner): Remove `std::monostate` once all uses of the default + // constructor are gone, and then add a public `Visit` method. + std::variant<std::monostate, ParameterizedItem, InnerList> value_; }; using DictionaryMember = std::pair<std::string, ParameterizedMember>;
diff --git a/quiche/common/structured_headers_test.cc b/quiche/common/structured_headers_test.cc index a139ac6..d2cfc7f 100644 --- a/quiche/common/structured_headers_test.cc +++ b/quiche/common/structured_headers_test.cc
@@ -824,6 +824,12 @@ EXPECT_EQ(member.GetWithParamsIfInnerList(), std::nullopt); EXPECT_EQ(std::as_const(member).GetWithParamsIfInnerList(), std::nullopt); + + EXPECT_FALSE(member.GetIfItem()); + EXPECT_FALSE(std::as_const(member).GetIfItem()); + + EXPECT_FALSE(member.GetIfInnerList()); + EXPECT_FALSE(std::as_const(member).GetIfInnerList()); } { @@ -838,6 +844,12 @@ EXPECT_EQ(member.GetWithParamsIfInnerList(), std::nullopt); EXPECT_EQ(std::as_const(member).GetWithParamsIfInnerList(), std::nullopt); + + auto* item_ptr = member.GetIfItem(); + ASSERT_TRUE(item_ptr); + EXPECT_EQ(*item_ptr, ParameterizedItem(item, params)); + + EXPECT_FALSE(member.GetIfInnerList()); } { @@ -853,6 +865,12 @@ EXPECT_EQ(member.GetWithParamsIfInnerList(), std::pair(items, params)); EXPECT_EQ(std::as_const(member).GetWithParamsIfInnerList(), std::pair(items, params)); + + EXPECT_FALSE(member.GetIfItem()); + + auto* inner_list_ptr = member.GetIfInnerList(); + ASSERT_TRUE(inner_list_ptr); + EXPECT_EQ(*inner_list_ptr, InnerList(items, params)); } }
diff --git a/quiche/web_transport/web_transport_headers.cc b/quiche/web_transport/web_transport_headers.cc index 93c00d7..e96095b 100644 --- a/quiche/web_transport/web_transport_headers.cc +++ b/quiche/web_transport/web_transport_headers.cc
@@ -59,15 +59,15 @@ template <Item::ItemType kExpectedType> auto GetMember(auto&& member) { - auto item = member.GetWithParamsIfItem(); - using ReturnType = decltype(GetItem<kExpectedType>(item->first)); + auto item = member.GetIfItem(); + using ReturnType = decltype(GetItem<kExpectedType>(item->item)); - if (!item.has_value()) { + if (!item) { return ReturnType(absl::InvalidArgumentError(absl::StrCat( "Expected all members to be of type", ItemTypeToString(kExpectedType), ", found a nested list instead"))); } - return GetItem<kExpectedType>(item->first); + return GetItem<kExpectedType>(item->item); } ABSL_CONST_INIT std::array kInitHeaderFields{