Skip to content

Commit

Permalink
Add DynamicStructPatch::patchIfSet<Tag>(id) and replace the existing …
Browse files Browse the repository at this point in the history
…API.

Summary:
This diff also changed `patchIfSet(badge, id, patch)` to private, just like the static patch:

https://www.internalfb.com/code/fbsource/[39b979388a33844ecef35fedbb3c53bf67e97912]/fbcode/thrift/lib/cpp2/op/detail/StructPatch.h?lines=583-587

Reviewed By: thedavekwon

Differential Revision: D65082475

fbshipit-source-id: 3df7839b51051668a3b44db9788036db93c51c34
  • Loading branch information
TJ Yin authored and facebook-github-bot committed Oct 30, 2024
1 parent febc5b0 commit 2fa9188
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ TEST(PatchMergeTest, DynamicStructPatch) {

op::I32Patch foo;
foo += 1;
p.patchIfSet(badge, FieldId{1}, DynamicPatch{foo});
p.patchIfSet<type::i32_t>(badge, FieldId{1}).merge(foo);

Object obj;
obj[FieldId(1)].emplace_i32(3);
Expand All @@ -819,7 +819,7 @@ TEST(PatchMergeTest, DynamicStructPatch) {
obj[FieldId(1)].emplace_i32(3);

// patch becomes += 2
p.patchIfSet(badge, FieldId{1}, DynamicPatch{foo});
p.patchIfSet<type::i32_t>(badge, FieldId{1}).merge(foo);

p.apply(badge, obj);
EXPECT_EQ(obj[FieldId(1)].as_i32(), 5);
Expand All @@ -829,7 +829,7 @@ TEST(PatchMergeTest, DynamicStructPatch) {
// In dynamic patch, we only use Remove operation to remove field
// Clear operation will just set field to intrinsic default
foo.clear();
p.patchIfSet(badge, FieldId{1}, DynamicPatch{foo});
p.patchIfSet<type::i32_t>(badge, FieldId{1}).merge(foo);
p.apply(badge, obj);
EXPECT_EQ(obj[FieldId(1)].as_i32(), 0);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -929,7 +929,7 @@ DynamicUnionPatch DiffVisitorBase::diffUnion(
auto guard = folly::makeGuard([&] { pop(); });
auto subPatch = diff(badge, src.at(id), dst.at(id));
if (!subPatch.empty(badge)) {
patch.patchIfSet(badge, id, DynamicPatch{std::move(subPatch)});
patch.patchIfSet(badge, id).merge(badge, DynamicPatch{std::move(subPatch)});
}

return patch;
Expand Down Expand Up @@ -1002,15 +1002,15 @@ void DiffVisitorBase::diffField(
auto empty = emptyValue(field.getType());
auto subPatch = diff(badge, empty, field);
patch.ensure(badge, id, std::move(empty));
patch.patchIfSet(badge, id, DynamicPatch{std::move(subPatch)});
patch.patchIfSet(badge, id).merge(badge, DynamicPatch{std::move(subPatch)});
return;
}

pushField(id);
auto guard = folly::makeGuard([&] { pop(); });
auto subPatch = diff(badge, src.at(id), dst.at(id));
if (!subPatch.empty(badge)) {
patch.patchIfSet(badge, id, DynamicPatch{std::move(subPatch)});
patch.patchIfSet(badge, id).merge(badge, DynamicPatch{std::move(subPatch)});
}
}

Expand Down
29 changes: 22 additions & 7 deletions third-party/thrift/src/thrift/lib/thrift/detail/DynamicPatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -448,18 +448,27 @@ class DynamicStructurePatch {
: ensureStruct(id, std::move(v));
}

// patchIfSet
template <class Tag>
op::patch_type<Tag>& patchIfSet(detail::Badge badge, FieldId id) {
using Patch = op::patch_type<Tag>;
auto& patch = patchIfSet(badge, id);

// patch already has the correct type, return it directly.
if (auto p = patch.template get_if<Patch>(badge)) {
return *p;
}

// Use merge to change patch's type.
patch.merge(badge, DynamicPatch{Patch{}});
return *patch.template get_if<Patch>(badge);
}

DynamicPatch& patchIfSet(detail::Badge, FieldId id) {
ensurePatchable();
return ensure_.contains(id) ? patchAfter_[id] : patchPrior_[id];
}

template <class SubPatch>
DynamicPatch& patchIfSet(detail::Badge badge, FieldId id, SubPatch&& patch) {
auto& ret = patchIfSet(badge, id);
ret.merge(badge, std::forward<SubPatch>(patch));
return ret;
}

[[nodiscard]] bool empty(detail::Badge) const {
return !assign_ && !clear_ && remove_.empty() && ensure_.empty() &&
patchPrior_.empty() && patchAfter_.empty();
Expand All @@ -478,6 +487,12 @@ class DynamicStructurePatch {
template <class Self, class Visitor>
static void customVisitImpl(Self&& self, detail::Badge, Visitor&& v);

// Needed for merge(...). We can consider making this a public API.
template <class SubPatch>
void patchIfSet(detail::Badge badge, FieldId id, SubPatch&& patch) {
patchIfSet(badge, id).merge(badge, std::forward<SubPatch>(patch));
}

public:
FOLLY_FOR_EACH_THIS_OVERLOAD_IN_CLASS_BODY_DELEGATE(
customVisit, customVisitImpl);
Expand Down

0 comments on commit 2fa9188

Please sign in to comment.