Skip to content

Commit 9da8594

Browse files
Mizuchimeta-codesync[bot]
authored andcommitted
@cpp.Arena is only accepted on fields it can give a PMR type
Summary: The next diff gives `cpp.Arena` fields PMR C++ types, so `cpp.Arena` is now only accepted on fields that can get one: string, binary, list, set and map fields, with or without `cpp.Type` on any level, that have no default value, reference, `cpp.Lazy`, `folly::IOBuf` C++ type, or adapter on any level of their type. Before: `cpp.Arena` was accepted on every field except adapted ones, and did nothing. After: it is accepted only on the fields above, and is an error on any other field. On accepted fields it still does nothing until the next diff. Reviewed By: iahs Differential Revision: D129420857 fbshipit-source-id: f057e4ed67a8308b8c1e18dc67d6737373d78713
1 parent 8712cf2 commit 9da8594

4 files changed

Lines changed: 80 additions & 0 deletions

File tree

‎thrift/compiler/generate/cpp/util.cc‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <thrift/compiler/generate/cpp/util.h>
1818

1919
#include <algorithm>
20+
#include <cassert>
2021
#include <functional>
2122
#include <memory>
2223
#include <queue>
@@ -120,6 +121,38 @@ bool is_custom_type(const t_field& field) {
120121
is_custom_type(*field.type());
121122
}
122123

124+
namespace {
125+
bool has_adapted_level(const t_type& type) {
126+
if (t_typedef::get_first_structured_annotation_or_null(
127+
&type, kCppAdapterUri) != nullptr) {
128+
return true;
129+
}
130+
const t_type* true_type = type.get_true_type();
131+
assert(true_type != nullptr);
132+
if (const auto* list = true_type->try_as<t_list>()) {
133+
return has_adapted_level(*list->elem_type());
134+
}
135+
if (const auto* set = true_type->try_as<t_set>()) {
136+
return has_adapted_level(*set->elem_type());
137+
}
138+
if (const auto* map = true_type->try_as<t_map>()) {
139+
return has_adapted_level(*map->key_type()) ||
140+
has_adapted_level(*map->val_type());
141+
}
142+
return false;
143+
}
144+
} // namespace
145+
146+
bool is_arena_supported(const t_field& field) {
147+
const t_type* type = field.type()->get_true_type();
148+
assert(type != nullptr);
149+
return (type->is_string_or_binary() || type->is<t_container>()) &&
150+
field.default_value() == nullptr && !is_ref(&field) && !is_lazy(&field) &&
151+
!is_binary_iobuf(field) &&
152+
!field.has_structured_annotation(kCppAdapterUri) &&
153+
!has_adapted_level(*field.type());
154+
}
155+
123156
bool container_supports_incomplete_params(const t_type& type) {
124157
if (t_typedef::get_first_structured_annotation_or_null(
125158
&type,

‎thrift/compiler/generate/cpp/util.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ inline std::string get_qualified_name(const t_named& node) {
5858
bool is_custom_type(const t_field& field);
5959
bool is_custom_type(const t_type& type);
6060

61+
// Whether field-level `@cpp.Arena` can give the field a PMR C++ type.
62+
bool is_arena_supported(const t_field& field);
63+
6164
std::unordered_map<const t_type*, std::vector<const t_type*>>
6265
gen_dependency_graph(
6366
const t_program* program, const std::vector<const t_type*>& types);

‎thrift/compiler/sema/standard_validator.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1425,6 +1425,10 @@ void validate_cpp_arena_field(sema_context& ctx, const t_field& field) {
14251425
}
14261426
if (adapter != nullptr) {
14271427
report_cpp_arena_on_adapted(ctx, "field", field.name(), *adapter);
1428+
return;
1429+
}
1430+
if (!cpp2::is_arena_supported(field)) {
1431+
ctx.error("`@cpp.Arena` is not supported on field `{}`.", field.name());
14281432
}
14291433
}
14301434

‎thrift/compiler/test/compiler_test.cc‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3457,6 +3457,46 @@ TEST(CompilerTest, cpp_arena_adapted) {
34573457
)");
34583458
}
34593459

3460+
TEST(CompilerTest, cpp_arena_unsupported_fields) {
3461+
check_compile(R"(
3462+
package "facebook.com/thrift/test"
3463+
include "thrift/annotation/cpp.thrift"
3464+
include "thrift/annotation/thrift.thrift"
3465+
3466+
@cpp.Adapter{name = "::my::Adapter"}
3467+
typedef string AdaptedString
3468+
@cpp.Type{template = "std::deque"}
3469+
typedef list<string> Deque
3470+
3471+
struct Foo {
3472+
@cpp.Arena
3473+
1: i32 number;
3474+
# expected-error@-2: `@cpp.Arena` is not supported on field `number`.
3475+
@cpp.Arena
3476+
2: string with_default = "default";
3477+
# expected-error@-2: `@cpp.Arena` is not supported on field `with_default`.
3478+
@cpp.Arena
3479+
@thrift.Box
3480+
3: optional string boxed;
3481+
# expected-error@-3: `@cpp.Arena` is not supported on field `boxed`.
3482+
@cpp.Arena
3483+
@cpp.Lazy
3484+
4: list<string> lazy;
3485+
# expected-error@-3: `@cpp.Arena` is not supported on field `lazy`.
3486+
@cpp.Arena
3487+
@cpp.Type{name = "::folly::IOBuf"}
3488+
5: binary iobuf;
3489+
# expected-error@-3: `@cpp.Arena` is not supported on field `iobuf`.
3490+
@cpp.Arena
3491+
6: list<AdaptedString> adapted_elements;
3492+
# expected-error@-2: `@cpp.Arena` is not supported on field `adapted_elements`.
3493+
@cpp.Arena
3494+
@cpp.Type{template = "std::unordered_map"}
3495+
7: map<string, Deque> custom;
3496+
}
3497+
)");
3498+
}
3499+
34603500
TEST(CompilerTest, cpp_use_allocator_ref_fields) {
34613501
check_compile(R"(
34623502
package "facebook.com/thrift/test"

0 commit comments

Comments
 (0)