Skip to content

Commit 8d2bf66

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
Refactor: parameter order for ConvertTypeSpecToType
For internal consistency (output parameter last). PiperOrigin-RevId: 950948076
1 parent e9e97ee commit 8d2bf66

5 files changed

Lines changed: 37 additions & 35 deletions

File tree

‎common/signature.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,7 @@ absl::StatusOr<TypeSpec> ParseTypeSpec(std::string_view signature) {
634634
absl::StatusOr<Type> ParseType(std::string_view signature, google::protobuf::Arena* arena,
635635
const google::protobuf::DescriptorPool& pool) {
636636
CEL_ASSIGN_OR_RETURN(auto type_spec, ParseTypeSpec(signature));
637-
return cel::ConvertTypeSpecToType(type_spec, arena, pool);
637+
return cel::ConvertTypeSpecToType(type_spec, pool, arena);
638638
}
639639

640640
} // namespace cel

‎common/signature_test.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ TEST_P(TypeSignatureTest, TypeSignature) {
8585
EXPECT_THAT(signature, IsOkAndHolds(param.expected_signature));
8686

8787
absl::StatusOr<Type> type = ConvertTypeSpecToType(
88-
param.type, GetTestArena(), *GetTestingDescriptorPool());
88+
param.type, *GetTestingDescriptorPool(), GetTestArena());
8989
ASSERT_THAT(type, ::absl_testing::IsOk());
9090
EXPECT_THAT(MakeTypeSignature(*type),
9191
IsOkAndHolds(param.expected_signature));
@@ -285,9 +285,10 @@ TEST_P(TypeSignatureTest, ParseTypeCheck) {
285285
auto parsed = ParseType(param.expected_signature, GetTestArena(),
286286
*GetTestingDescriptorPool());
287287
ASSERT_THAT(parsed, ::absl_testing::IsOk());
288-
ASSERT_OK_AND_ASSIGN(auto expected_type,
289-
ConvertTypeSpecToType(param.type, GetTestArena(),
290-
*GetTestingDescriptorPool()));
288+
ASSERT_OK_AND_ASSIGN(
289+
auto expected_type,
290+
ConvertTypeSpecToType(param.type, *GetTestingDescriptorPool(),
291+
GetTestArena()));
291292
VerifyTypesEqual(*parsed, expected_type);
292293
}
293294
}

‎common/type_spec_resolver.cc‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@
3333
namespace cel {
3434

3535
absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
36-
google::protobuf::Arena* arena,
37-
const google::protobuf::DescriptorPool& pool) {
36+
const google::protobuf::DescriptorPool& pool,
37+
google::protobuf::Arena* arena) {
3838
if (type_spec.has_null()) return Type(NullType{});
3939
if (type_spec.has_dyn()) return Type(DynType{});
4040

@@ -94,7 +94,7 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
9494
if (type_spec.list_type().elem_type().is_specified()) {
9595
CEL_ASSIGN_OR_RETURN(
9696
elem_type, ConvertTypeSpecToType(type_spec.list_type().elem_type(),
97-
arena, pool));
97+
pool, arena));
9898
}
9999
return Type(ListType(arena, elem_type));
100100
}
@@ -104,14 +104,14 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
104104
if (type_spec.map_type().key_type().is_specified()) {
105105
CEL_ASSIGN_OR_RETURN(
106106
key_type,
107-
ConvertTypeSpecToType(type_spec.map_type().key_type(), arena, pool));
107+
ConvertTypeSpecToType(type_spec.map_type().key_type(), pool, arena));
108108
}
109109

110110
Type value_type;
111111
if (type_spec.map_type().value_type().is_specified()) {
112112
CEL_ASSIGN_OR_RETURN(
113113
value_type, ConvertTypeSpecToType(type_spec.map_type().value_type(),
114-
arena, pool));
114+
pool, arena));
115115
}
116116
return Type(MapType(arena, key_type, value_type));
117117
}
@@ -122,13 +122,13 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
122122
if (func_spec.result_type().is_specified()) {
123123
CEL_ASSIGN_OR_RETURN(
124124
result_type,
125-
ConvertTypeSpecToType(func_spec.result_type(), arena, pool));
125+
ConvertTypeSpecToType(func_spec.result_type(), pool, arena));
126126
}
127127
std::vector<Type> arg_types;
128128
arg_types.reserve(func_spec.arg_types().size());
129129
for (const auto& arg_spec : func_spec.arg_types()) {
130130
CEL_ASSIGN_OR_RETURN(auto arg_type,
131-
ConvertTypeSpecToType(arg_spec, arena, pool));
131+
ConvertTypeSpecToType(arg_spec, pool, arena));
132132
arg_types.push_back(std::move(arg_type));
133133
}
134134
return Type(FunctionType(arena, result_type, arg_types));
@@ -178,7 +178,7 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
178178
std::vector<Type> params;
179179
for (const auto& param_spec : type_spec.abstract_type().parameter_types()) {
180180
CEL_ASSIGN_OR_RETURN(auto param,
181-
ConvertTypeSpecToType(param_spec, arena, pool));
181+
ConvertTypeSpecToType(param_spec, pool, arena));
182182
params.push_back(std::move(param));
183183
}
184184
auto* allocated_name = google::protobuf::Arena::Create<std::string>(arena, name);
@@ -187,7 +187,7 @@ absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
187187

188188
if (type_spec.has_type()) {
189189
CEL_ASSIGN_OR_RETURN(auto contained_type,
190-
ConvertTypeSpecToType(type_spec.type(), arena, pool));
190+
ConvertTypeSpecToType(type_spec.type(), pool, arena));
191191
return Type(TypeType(arena, contained_type));
192192
}
193193

‎common/type_spec_resolver.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ namespace cel {
2929
// properties of the type when used in CEL. Returns a status with code
3030
// `InvalidArgument` if the input cannot be resolved to a type.
3131
absl::StatusOr<Type> ConvertTypeSpecToType(const TypeSpec& type_spec,
32-
google::protobuf::Arena* arena,
33-
const google::protobuf::DescriptorPool& pool);
32+
const google::protobuf::DescriptorPool& pool,
33+
google::protobuf::Arena* arena);
3434

3535
// Resolves a `cel::Type` to a `cel::TypeSpec`.
3636
absl::StatusOr<TypeSpec> ConvertTypeToTypeSpec(const Type& type);

‎common/type_spec_resolver_test.cc‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -49,15 +49,15 @@ google::protobuf::Arena* GetTestArena() {
4949
TEST(TypeSpecResolverTest, NullTypeSpec) {
5050
TypeSpec spec(NullTypeSpec{});
5151
auto t =
52-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
52+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
5353
ASSERT_THAT(t, IsOk());
5454
EXPECT_TRUE(t->IsNull());
5555
}
5656

5757
TEST(TypeSpecResolverTest, DynTypeSpec) {
5858
TypeSpec spec(DynTypeSpec{});
5959
auto t =
60-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
60+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
6161
ASSERT_THAT(t, IsOk());
6262
EXPECT_TRUE(t->IsDyn());
6363
}
@@ -66,8 +66,9 @@ using ConversionTest = testing::TestWithParam<std::tuple<TypeSpec, TypeKind>>;
6666

6767
TEST_P(ConversionTest, TestTypeSpecConversion) {
6868
ASSERT_OK_AND_ASSIGN(
69-
auto t, ConvertTypeSpecToType(std::get<0>(GetParam()), GetTestArena(),
70-
*GetTestingDescriptorPool()));
69+
auto t,
70+
ConvertTypeSpecToType(std::get<0>(GetParam()),
71+
*GetTestingDescriptorPool(), GetTestArena()));
7172
EXPECT_EQ(t.kind(), std::get<1>(GetParam()));
7273
EXPECT_THAT(ConvertTypeToTypeSpec(t), IsOkAndHolds(std::get<0>(GetParam())));
7374
}
@@ -103,7 +104,7 @@ TEST(TypeSpecResolverTest, ListTypeConversion) {
103104
auto elem = std::make_unique<TypeSpec>(PrimitiveType::kInt64);
104105
TypeSpec spec(ListTypeSpec(std::move(elem)));
105106
auto t =
106-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
107+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
107108
ASSERT_THAT(t, IsOk());
108109
EXPECT_TRUE(t->IsList());
109110
EXPECT_TRUE(t->GetList().element().IsInt());
@@ -116,7 +117,7 @@ TEST(TypeSpecResolverTest, MapTypeConversion) {
116117
auto val = std::make_unique<TypeSpec>(PrimitiveType::kBytes);
117118
TypeSpec spec(MapTypeSpec(std::move(key), std::move(val)));
118119
auto t =
119-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
120+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
120121
ASSERT_THAT(t, IsOk());
121122
EXPECT_TRUE(t->IsMap());
122123
EXPECT_TRUE(t->GetMap().key().IsString());
@@ -131,7 +132,7 @@ TEST(TypeSpecResolverTest, FunctionTypeConversion) {
131132
args.push_back(TypeSpec(PrimitiveType::kString));
132133
TypeSpec spec(FunctionTypeSpec(std::move(result), std::move(args)));
133134
auto t =
134-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
135+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
135136
ASSERT_THAT(t, IsOk());
136137
EXPECT_TRUE(t->IsFunction());
137138
EXPECT_EQ(t->GetFunction().args().size(), 1);
@@ -143,7 +144,7 @@ TEST(TypeSpecResolverTest, FunctionTypeConversion) {
143144
TEST(TypeSpecResolverTest, TypeParamConversion) {
144145
TypeSpec spec(ParamTypeSpec("T"));
145146
auto t =
146-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
147+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
147148
ASSERT_THAT(t, IsOk());
148149
EXPECT_TRUE(t->IsTypeParam());
149150
EXPECT_EQ(t->GetTypeParam().name(), "T");
@@ -155,7 +156,7 @@ TEST(TypeSpecResolverTest, MessageTypeConversion) {
155156
TypeSpec spec(
156157
AbstractType("cel.expr.conformance.proto3.TestAllTypes", /*params=*/{}));
157158
auto t =
158-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
159+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
159160
ASSERT_THAT(t, IsOk());
160161
EXPECT_TRUE(t->IsMessage());
161162
EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes");
@@ -171,7 +172,7 @@ TEST(TypeSpecResolverTest, MessageTypeWithParamsError) {
171172
TypeSpec spec(AbstractType("cel.expr.conformance.proto3.TestAllTypes",
172173
std::move(params)));
173174
auto t =
174-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
175+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
175176
EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
176177
HasSubstr("cannot have type parameters")));
177178
}
@@ -181,7 +182,7 @@ TEST(TypeSpecResolverTest, UnresolvedAbstractTypeFallbackToOpaque) {
181182
params.push_back(TypeSpec(PrimitiveType::kInt64));
182183
TypeSpec spec(AbstractType("my.custom.OpaqueType", std::move(params)));
183184
auto t =
184-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
185+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
185186
ASSERT_THAT(t, IsOk());
186187
EXPECT_TRUE(t->IsOpaque());
187188
EXPECT_EQ(t->name(), "my.custom.OpaqueType");
@@ -196,7 +197,7 @@ TEST(TypeSpecResolverTest, OptionalType) {
196197
params.push_back(TypeSpec(PrimitiveType::kInt64));
197198
TypeSpec spec(AbstractType("optional_type", std::move(params)));
198199
auto t =
199-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
200+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
200201
ASSERT_THAT(t, IsOk());
201202
EXPECT_TRUE(t->IsOpaque());
202203
EXPECT_EQ(t->name(), "optional_type");
@@ -211,7 +212,7 @@ TEST(TypeSpecResolverTest, TypeTypeConversion) {
211212
auto nested = std::make_unique<TypeSpec>(PrimitiveType::kInt64);
212213
TypeSpec spec(std::move(nested));
213214
auto t =
214-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
215+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
215216
ASSERT_THAT(t, IsOk());
216217
EXPECT_TRUE(t->IsType());
217218
EXPECT_TRUE(t->GetType().GetType().IsInt());
@@ -222,7 +223,7 @@ TEST(TypeSpecResolverTest, TypeTypeConversion) {
222223
TEST(TypeSpecResolverTest, ErrorTypeConversion) {
223224
TypeSpec spec(ErrorTypeSpec::kValue);
224225
auto t =
225-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
226+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
226227
ASSERT_THAT(t, IsOk());
227228
EXPECT_TRUE(t->IsError());
228229
ASSERT_OK_AND_ASSIGN(auto spec2, ConvertTypeToTypeSpec(*t));
@@ -232,7 +233,7 @@ TEST(TypeSpecResolverTest, ErrorTypeConversion) {
232233
TEST(TypeSpecResolverTest, MessageTypeSpecConversion) {
233234
TypeSpec spec(MessageTypeSpec("cel.expr.conformance.proto3.TestAllTypes"));
234235
auto t =
235-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
236+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
236237
ASSERT_THAT(t, IsOk());
237238
EXPECT_TRUE(t->IsMessage());
238239
EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes");
@@ -243,7 +244,7 @@ TEST(TypeSpecResolverTest, MessageTypeSpecConversion) {
243244
TEST(TypeSpecResolverTest, MessageTypeSpecNotFoundError) {
244245
TypeSpec spec(MessageTypeSpec("cel.expr.conformance.proto3.NonExistentType"));
245246
auto t =
246-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
247+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
247248
EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
248249
HasSubstr("not found in descriptor pool")));
249250
}
@@ -252,7 +253,7 @@ TEST(TypeSpecResolverTest, EnumTypeConversion) {
252253
TypeSpec spec(AbstractType(
253254
"cel.expr.conformance.proto3.TestAllTypes.NestedEnum", /*params=*/{}));
254255
auto t =
255-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
256+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
256257
ASSERT_THAT(t, IsOk());
257258
EXPECT_TRUE(t->IsEnum());
258259
EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes.NestedEnum");
@@ -267,15 +268,15 @@ TEST(TypeSpecResolverTest, EnumTypeWithParamsError) {
267268
AbstractType("cel.expr.conformance.proto3.TestAllTypes.NestedEnum",
268269
std::move(params)));
269270
auto t =
270-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
271+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
271272
EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
272273
HasSubstr("cannot have type parameters")));
273274
}
274275

275276
TEST(TypeSpecResolverTest, UnknownTypeSpecKindError) {
276277
TypeSpec spec;
277278
auto t =
278-
ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool());
279+
ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena());
279280
EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument,
280281
HasSubstr("Unknown TypeSpec kind")));
281282
}

0 commit comments

Comments
 (0)