Skip to content

Commit 91d4b1b

Browse files
sammy-SCfacebook-github-bot
sammy-SC
authored and
facebook-github-bot
committed
pass RawProps by rvalue (facebook#41604)
Summary: Pull Request resolved: facebook#41604 changelog: [internal] Remove const requirement when using RawProps and prefer passing it by rvalue. Reviewed By: javache Differential Revision: D51471667 fbshipit-source-id: 479bcebe642e168ff1f110e7b2bfaf20a3e82821
1 parent 951efc8 commit 91d4b1b

File tree

11 files changed

+187
-75
lines changed

11 files changed

+187
-75
lines changed

‎packages/react-native/ReactCommon/react/renderer/components/unimplementedview/UnimplementedViewComponentDescriptor.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ ComponentName UnimplementedViewComponentDescriptor::getComponentName() const {
2121
Props::Shared UnimplementedViewComponentDescriptor::cloneProps(
2222
const PropsParserContext& context,
2323
const Props::Shared& props,
24-
const RawProps& rawProps) const {
24+
RawProps rawProps) const {
2525
auto clonedProps =
2626
ConcreteComponentDescriptor<UnimplementedViewShadowNode>::cloneProps(
27-
context, props, rawProps);
27+
context, props, std::move(rawProps));
2828

2929
// We have to clone `Props` object one more time to make sure that we have
3030
// an unshared (and non-`const`) copy of it which we can mutate.
@@ -33,7 +33,7 @@ Props::Shared UnimplementedViewComponentDescriptor::cloneProps(
3333
auto unimplementedViewProps = std::make_shared<UnimplementedViewProps>(
3434
context,
3535
static_cast<const UnimplementedViewProps&>(*clonedProps),
36-
emptyRawProps);
36+
std::move(emptyRawProps));
3737

3838
unimplementedViewProps->setComponentName(getComponentName());
3939
return unimplementedViewProps;

‎packages/react-native/ReactCommon/react/renderer/components/unimplementedview/UnimplementedViewComponentDescriptor.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class UnimplementedViewComponentDescriptor final
3535
Props::Shared cloneProps(
3636
const PropsParserContext& context,
3737
const Props::Shared& props,
38-
const RawProps& rawProps) const override;
38+
RawProps rawProps) const override;
3939
};
4040

4141
} // namespace facebook::react

‎packages/react-native/ReactCommon/react/renderer/core/ComponentDescriptor.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ class ComponentDescriptor {
105105
virtual Props::Shared cloneProps(
106106
const PropsParserContext& context,
107107
const Props::Shared& props,
108-
const RawProps& rawProps) const = 0;
108+
RawProps rawProps) const = 0;
109109

110110
/*
111111
* Create an initial State object that represents (and contains) an initial

‎packages/react-native/ReactCommon/react/renderer/core/ConcreteComponentDescriptor.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ class ConcreteComponentDescriptor : public ComponentDescriptor {
9595
virtual Props::Shared cloneProps(
9696
const PropsParserContext& context,
9797
const Props::Shared& props,
98-
const RawProps& rawProps) const override {
98+
RawProps rawProps) const override {
9999
// Optimization:
100100
// Quite often nodes are constructed with default/empty props: the base
101101
// `props` object is `null` (there no base because it's not cloning) and the

‎packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ RawProps::RawProps(jsi::Runtime& runtime, const jsi::Value& value) noexcept {
2626
return;
2727
}
2828

29-
mode_ = mode_ = Mode::JSI;
29+
mode_ = Mode::JSI;
3030
runtime_ = &runtime;
3131
value_ = jsi::Value(runtime, value);
3232
}
@@ -47,7 +47,28 @@ RawProps::RawProps(folly::dynamic dynamic) noexcept {
4747
dynamic_ = std::move(dynamic);
4848
}
4949

50-
void RawProps::parse(const RawPropsParser& parser) const noexcept {
50+
RawProps::RawProps(const RawProps& other) noexcept {
51+
mode_ = other.mode_;
52+
if (mode_ == Mode::JSI) {
53+
runtime_ = other.runtime_;
54+
value_ = jsi::Value(*runtime_, other.value_);
55+
} else if (mode_ == Mode::Dynamic) {
56+
dynamic_ = other.dynamic_;
57+
}
58+
}
59+
60+
RawProps& RawProps::operator=(const RawProps& other) noexcept {
61+
mode_ = other.mode_;
62+
if (mode_ == Mode::JSI) {
63+
runtime_ = other.runtime_;
64+
value_ = jsi::Value(*runtime_, other.value_);
65+
} else if (mode_ == Mode::Dynamic) {
66+
dynamic_ = other.dynamic_;
67+
}
68+
return *this;
69+
}
70+
71+
void RawProps::parse(const RawPropsParser& parser) noexcept {
5172
react_native_assert(parser_ == nullptr && "A parser was already assigned.");
5273
parser_ = &parser;
5374
parser.preparse(*this);

‎packages/react-native/ReactCommon/react/renderer/core/RawProps.h‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,12 @@ class RawProps final {
5050
*/
5151
RawProps(jsi::Runtime& runtime, const jsi::Value& value) noexcept;
5252

53+
explicit RawProps(const RawProps& rawProps) noexcept;
54+
RawProps& operator=(const RawProps& other) noexcept;
55+
56+
RawProps(RawProps&& other) noexcept = default;
57+
RawProps& operator=(RawProps&& other) noexcept = default;
58+
5359
/*
5460
* Creates an object with given `folly::dynamic` object.
5561
* Deprecated. Do not use.
@@ -58,19 +64,7 @@ class RawProps final {
5864
*/
5965
explicit RawProps(folly::dynamic dynamic) noexcept;
6066

61-
/*
62-
* Not moveable.
63-
*/
64-
RawProps(RawProps&& other) noexcept = delete;
65-
RawProps& operator=(RawProps&& other) noexcept = delete;
66-
67-
/*
68-
* Not copyable.
69-
*/
70-
RawProps(const RawProps& other) noexcept = delete;
71-
RawProps& operator=(const RawProps& other) noexcept = delete;
72-
73-
void parse(const RawPropsParser& parser) const noexcept;
67+
void parse(const RawPropsParser& parser) noexcept;
7468

7569
/*
7670
* Deprecated. Do not use.

‎packages/react-native/ReactCommon/react/renderer/core/tests/ComponentDescriptorTest.cpp‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,9 @@ TEST(ComponentDescriptorTest, createShadowNode) {
2626
ContextContainer contextContainer{};
2727
PropsParserContext parserContext{-1, contextContainer};
2828

29-
const auto& raw = RawProps(folly::dynamic::object("nativeID", "abc"));
30-
Props::Shared props = descriptor->cloneProps(parserContext, nullptr, raw);
29+
auto rawProps = RawProps(folly::dynamic::object("nativeID", "abc"));
30+
Props::Shared props =
31+
descriptor->cloneProps(parserContext, nullptr, std::move(rawProps));
3132

3233
auto family = descriptor->createFamily(ShadowNodeFamilyFragment{
3334
/* .tag = */ 9,
@@ -58,8 +59,9 @@ TEST(ComponentDescriptorTest, cloneShadowNode) {
5859
ContextContainer contextContainer{};
5960
PropsParserContext parserContext{-1, contextContainer};
6061

61-
const auto& raw = RawProps(folly::dynamic::object("nativeID", "abc"));
62-
Props::Shared props = descriptor->cloneProps(parserContext, nullptr, raw);
62+
auto rawProps = RawProps(folly::dynamic::object("nativeID", "abc"));
63+
Props::Shared props =
64+
descriptor->cloneProps(parserContext, nullptr, std::move(rawProps));
6365
auto family = descriptor->createFamily(ShadowNodeFamilyFragment{
6466
/* .tag = */ 9,
6567
/* .surfaceId = */ 1,
@@ -91,8 +93,9 @@ TEST(ComponentDescriptorTest, appendChild) {
9193
ContextContainer contextContainer{};
9294
PropsParserContext parserContext{-1, contextContainer};
9395

94-
const auto& raw = RawProps(folly::dynamic::object("nativeID", "abc"));
95-
Props::Shared props = descriptor->cloneProps(parserContext, nullptr, raw);
96+
auto rawProps = RawProps(folly::dynamic::object("nativeID", "abc"));
97+
Props::Shared props =
98+
descriptor->cloneProps(parserContext, nullptr, std::move(rawProps));
9699
auto family1 = descriptor->createFamily(ShadowNodeFamilyFragment{
97100
/* .tag = */ 1,
98101
/* .surfaceId = */ 1,

‎packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp‎

Lines changed: 107 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <memory>
99

1010
#include <gtest/gtest.h>
11+
#include <hermes/hermes.h>
1112
#include <react/debug/flags.h>
1213
#include <react/renderer/core/ConcreteShadowNode.h>
1314
#include <react/renderer/core/PropsParserContext.h>
@@ -16,6 +17,7 @@
1617

1718
#include "TestComponent.h"
1819

20+
using namespace facebook;
1921
using namespace facebook::react;
2022

2123
class PropsSingleFloat : public Props {
@@ -151,7 +153,7 @@ TEST(RawPropsTest, handleProps) {
151153
ContextContainer contextContainer{};
152154
PropsParserContext parserContext{-1, contextContainer};
153155

154-
const auto& raw = RawProps(folly::dynamic::object("nativeID", "abc"));
156+
auto raw = RawProps(folly::dynamic::object("nativeID", "abc"));
155157
auto parser = RawPropsParser();
156158
parser.prepare<Props>();
157159
raw.parse(parser);
@@ -168,7 +170,7 @@ TEST(RawPropsTest, handleRawPropsSingleString) {
168170
ContextContainer contextContainer{};
169171
PropsParserContext parserContext{-1, contextContainer};
170172

171-
const auto& raw = RawProps(folly::dynamic::object("nativeID", "abc"));
173+
auto raw = RawProps(folly::dynamic::object("nativeID", "abc"));
172174
auto parser = RawPropsParser();
173175
parser.prepare<Props>();
174176
raw.parse(parser);
@@ -182,8 +184,7 @@ TEST(RawPropsTest, handleRawPropsSingleFloat) {
182184
ContextContainer contextContainer{};
183185
PropsParserContext parserContext{-1, contextContainer};
184186

185-
const auto& raw =
186-
RawProps(folly::dynamic::object("floatValue", (float)42.42));
187+
auto raw = RawProps(folly::dynamic::object("floatValue", (float)42.42));
187188
auto parser = RawPropsParser();
188189
parser.prepare<PropsSingleFloat>();
189190
raw.parse(parser);
@@ -197,8 +198,7 @@ TEST(RawPropsTest, handleRawPropsSingleDouble) {
197198
ContextContainer contextContainer{};
198199
PropsParserContext parserContext{-1, contextContainer};
199200

200-
const auto& raw =
201-
RawProps(folly::dynamic::object("doubleValue", (double)42.42));
201+
auto raw = RawProps(folly::dynamic::object("doubleValue", (double)42.42));
202202
auto parser = RawPropsParser();
203203
parser.prepare<PropsSingleDouble>();
204204
raw.parse(parser);
@@ -212,7 +212,7 @@ TEST(RawPropsTest, handleRawPropsSingleInt) {
212212
ContextContainer contextContainer{};
213213
PropsParserContext parserContext{-1, contextContainer};
214214

215-
const auto& raw = RawProps(folly::dynamic::object("intValue", (int)42.42));
215+
auto raw = RawProps(folly::dynamic::object("intValue", (int)42.42));
216216
auto parser = RawPropsParser();
217217
parser.prepare<PropsSingleInt>();
218218
raw.parse(parser);
@@ -226,7 +226,7 @@ TEST(RawPropsTest, handleRawPropsSingleIntGetManyTimes) {
226226
ContextContainer contextContainer{};
227227
PropsParserContext parserContext{-1, contextContainer};
228228

229-
const auto& raw = RawProps(folly::dynamic::object("intValue", (int)42.42));
229+
auto raw = RawProps(folly::dynamic::object("intValue", (int)42.42));
230230
auto parser = RawPropsParser();
231231
parser.prepare<PropsSingleInt>();
232232
raw.parse(parser);
@@ -240,7 +240,7 @@ TEST(RawPropsTest, handleRawPropsPrimitiveTypes) {
240240
ContextContainer contextContainer{};
241241
PropsParserContext parserContext{-1, contextContainer};
242242

243-
const auto& raw = RawProps(
243+
auto raw = RawProps(
244244
folly::dynamic::object("intValue", (int)42)("doubleValue", (double)17.42)(
245245
"floatValue",
246246
(float)66.67)("stringValue", "helloworld")("boolValue", true));
@@ -262,7 +262,7 @@ TEST(RawPropsTest, handleRawPropsPrimitiveTypesGetTwice) {
262262
ContextContainer contextContainer{};
263263
PropsParserContext parserContext{-1, contextContainer};
264264

265-
const auto& raw = RawProps(
265+
auto raw = RawProps(
266266
folly::dynamic::object("intValue", (int)42)("doubleValue", (double)17.42)(
267267
"floatValue",
268268
(float)66.67)("stringValue", "helloworld")("boolValue", true));
@@ -292,7 +292,7 @@ TEST(RawPropsTest, handleRawPropsPrimitiveTypesGetOutOfOrder) {
292292
ContextContainer contextContainer{};
293293
PropsParserContext parserContext{-1, contextContainer};
294294

295-
const auto& raw = RawProps(
295+
auto raw = RawProps(
296296
folly::dynamic::object("intValue", (int)42)("doubleValue", (double)17.42)(
297297
"floatValue",
298298
(float)66.67)("stringValue", "helloworld")("boolValue", true));
@@ -322,7 +322,7 @@ TEST(RawPropsTest, handleRawPropsPrimitiveTypesIncomplete) {
322322
ContextContainer contextContainer{};
323323
PropsParserContext parserContext{-1, contextContainer};
324324

325-
const auto& raw = RawProps(folly::dynamic::object("intValue", (int)42));
325+
auto raw = RawProps(folly::dynamic::object("intValue", (int)42));
326326

327327
auto parser = RawPropsParser();
328328
parser.prepare<PropsPrimitiveTypes>();
@@ -342,7 +342,7 @@ TEST(RawPropsTest, handleRawPropsPrimitiveTypesIncorrectLookup) {
342342
ContextContainer contextContainer{};
343343
PropsParserContext parserContext{-1, contextContainer};
344344

345-
const auto& raw = RawProps(folly::dynamic::object("intValue", (int)42));
345+
auto raw = RawProps(folly::dynamic::object("intValue", (int)42));
346346

347347
auto parser = RawPropsParser();
348348
parser.prepare<PropsPrimitiveTypes>();
@@ -360,7 +360,7 @@ TEST(RawPropsTest, handlePropsMultiLookup) {
360360
ContextContainer contextContainer{};
361361
PropsParserContext parserContext{-1, contextContainer};
362362

363-
const auto& raw = RawProps(folly::dynamic::object("floatValue", (float)10.0));
363+
auto raw = RawProps(folly::dynamic::object("floatValue", (float)10.0));
364364
auto parser = RawPropsParser();
365365
parser.prepare<PropsMultiLookup>();
366366
raw.parse(parser);
@@ -374,3 +374,96 @@ TEST(RawPropsTest, handlePropsMultiLookup) {
374374
EXPECT_NEAR(props->floatValue, 10.0, 0.00001);
375375
EXPECT_NEAR(props->derivedFloatValue, 20.0, 0.00001);
376376
}
377+
378+
TEST(RawPropsTest, copyDynamicRawProps) {
379+
ContextContainer contextContainer{};
380+
PropsParserContext parserContext{-1, contextContainer};
381+
382+
auto rawProps = RawProps(folly::dynamic::object("floatValue", (float)10.0));
383+
384+
auto copy = RawProps(rawProps);
385+
386+
EXPECT_FALSE(copy.isEmpty());
387+
388+
auto parser = RawPropsParser();
389+
parser.prepare<PropsMultiLookup>();
390+
391+
rawProps.parse(parser);
392+
copy.parse(parser);
393+
394+
auto originalProps = std::make_shared<PropsMultiLookup>(
395+
parserContext, PropsMultiLookup(), rawProps);
396+
auto copyProps = std::make_shared<PropsMultiLookup>(
397+
parserContext, PropsMultiLookup(), copy);
398+
399+
// Props are not sealed after applying raw props.
400+
EXPECT_FALSE(copyProps->getSealed());
401+
402+
EXPECT_NEAR(copyProps->floatValue, originalProps->floatValue, 0.00001);
403+
EXPECT_NEAR(
404+
copyProps->derivedFloatValue, originalProps->derivedFloatValue, 0.00001);
405+
}
406+
407+
TEST(RawPropsTest, copyEmptyRawProps) {
408+
ContextContainer contextContainer{};
409+
PropsParserContext parserContext{-1, contextContainer};
410+
411+
auto rawProps = RawProps();
412+
413+
auto copy = RawProps(rawProps);
414+
415+
EXPECT_TRUE(rawProps.isEmpty());
416+
EXPECT_TRUE(copy.isEmpty());
417+
418+
EXPECT_TRUE(((folly::dynamic)copy).empty());
419+
}
420+
421+
TEST(RawPropsTest, copyNullJSIRawProps) {
422+
auto runtime = facebook::hermes::makeHermesRuntime();
423+
424+
ContextContainer contextContainer{};
425+
PropsParserContext parserContext{-1, contextContainer};
426+
427+
auto rawProps = RawProps(*runtime, jsi::Value::null());
428+
429+
auto copy = RawProps(rawProps);
430+
431+
EXPECT_TRUE(rawProps.isEmpty());
432+
EXPECT_TRUE(copy.isEmpty());
433+
434+
EXPECT_TRUE(((folly::dynamic)copy).empty());
435+
}
436+
437+
TEST(RawPropsTest, copyJSIRawProps) {
438+
auto runtime = facebook::hermes::makeHermesRuntime();
439+
440+
ContextContainer contextContainer{};
441+
PropsParserContext parserContext{-1, contextContainer};
442+
443+
auto object = jsi::Object(*runtime);
444+
object.setProperty(*runtime, "floatValue", 10.0);
445+
446+
auto rawProps = RawProps(*runtime, jsi::Value(*runtime, object));
447+
auto copy = RawProps(rawProps);
448+
449+
EXPECT_FALSE(rawProps.isEmpty());
450+
EXPECT_FALSE(copy.isEmpty());
451+
452+
auto parser = RawPropsParser();
453+
parser.prepare<PropsMultiLookup>();
454+
455+
rawProps.parse(parser);
456+
copy.parse(parser);
457+
458+
auto originalProps = std::make_shared<PropsMultiLookup>(
459+
parserContext, PropsMultiLookup(), rawProps);
460+
auto copyProps = std::make_shared<PropsMultiLookup>(
461+
parserContext, PropsMultiLookup(), copy);
462+
463+
// Props are not sealed after applying raw props.
464+
EXPECT_FALSE(copyProps->getSealed());
465+
466+
EXPECT_NEAR(copyProps->floatValue, originalProps->floatValue, 0.00001);
467+
EXPECT_NEAR(
468+
copyProps->derivedFloatValue, originalProps->derivedFloatValue, 0.00001);
469+
}

0 commit comments

Comments
 (0)