Skip to content

Commit 81a97de

Browse files
sammy-SCfacebook-github-bot
authored andcommitted
Prevent type conversion in Differentiator
Summary: changelog: [internal] Prevents 2 type converions: 1. int <-> size_t 2. int <-> int32_t # Why is using size_t better when working with indexes. ## 1. Type conversion isn't for free. Take this example ``` size_t calculate(int number) { return number + 1; } ``` It generates following assembly (generated with armv8-a clang 10.0.0): ``` calculate(int): // calculate(int) sub sp, sp, microsoft#16 // =16 str w0, [sp, microsoft#12] ldr w8, [sp, microsoft#12] add w9, w8, #1 // =1 mov w8, w9 sxtw x0, w8 add sp, sp, microsoft#16 // =16 ret ``` That's 9 instructions. If we get rid of type conversion: ``` size_t calculate(size_t number) { return number + 1; } ``` Assembly (generated with armv8-a clang 10.0.0): ``` calculate(unsigned long): // calculate(unsigned long) sub sp, sp, microsoft#16 // =16 str x0, [sp, microsoft#8] ldr x8, [sp, microsoft#8] add x0, x8, #1 // =1 add sp, sp, microsoft#16 // =16 ret ``` Compiler now produces only 7 instructions. ## Semantics When using int for indexing, the type doesn't say much. By using `size_t`, just by looking at the type, it gives the reader more information about where it is coming from. Reviewed By: JoshuaGross Differential Revision: D24332248 fbshipit-source-id: 87ef982829ec14906ed9e002ea2e875fda4a0cd8
1 parent b70152c commit 81a97de

File tree

2 files changed

+25
-25
lines changed

2 files changed

+25
-25
lines changed

ReactCommon/react/renderer/mounting/Differentiator.cpp

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ ShadowViewNodePair::List sliceChildShadowNodeViewPairsV2(
251251
reorderInPlaceIfNeeded(pairList);
252252

253253
// Set list and mountIndex for each after reordering
254-
int mountIndex = 0;
254+
size_t mountIndex = 0;
255255
for (auto &child : pairList) {
256256
child.mountIndex = (child.isConcreteView ? mountIndex++ : -1);
257257
}
@@ -376,7 +376,7 @@ static void calculateShadowViewMutationsFlattener(
376376
<< " [" << node.shadowView.tag << "]";
377377
LOG(ERROR) << "Differ Flattener Entry: Child Pairs: ";
378378
std::string strTreeChildPairs;
379-
for (int k = 0; k < treeChildren.size(); k++) {
379+
for (size_t k = 0; k < treeChildren.size(); k++) {
380380
strTreeChildPairs.append(std::to_string(treeChildren[k].shadowView.tag));
381381
strTreeChildPairs.append(treeChildren[k].isConcreteView ? "" : "'");
382382
strTreeChildPairs.append(treeChildren[k].flattened ? "*" : "");
@@ -410,7 +410,7 @@ static void calculateShadowViewMutationsFlattener(
410410
auto deletionCreationCandidatePairs =
411411
TinyMap<Tag, ShadowViewNodePair const *>{};
412412

413-
for (int index = 0;
413+
for (size_t index = 0;
414414
index < treeChildren.size() && index < treeChildren.size();
415415
index++) {
416416
// First, remove all children of the tree being flattened, or insert
@@ -530,7 +530,7 @@ static void calculateShadowViewMutationsFlattener(
530530
// this "else" block, including any annotations we put on them.
531531
auto newFlattenedNodes = sliceChildShadowNodeViewPairsV2(
532532
*newTreeNodePair.shadowNode, true);
533-
for (int i = 0; i < newFlattenedNodes.size(); i++) {
533+
for (size_t i = 0; i < newFlattenedNodes.size(); i++) {
534534
auto &newChild = newFlattenedNodes[i];
535535

536536
auto unvisitedOtherNodesIt =
@@ -582,7 +582,7 @@ static void calculateShadowViewMutationsFlattener(
582582
// this "else" block, including any annotations we put on them.
583583
auto oldFlattenedNodes = sliceChildShadowNodeViewPairsV2(
584584
*oldTreeNodePair.shadowNode, true);
585-
for (int i = 0; i < oldFlattenedNodes.size(); i++) {
585+
for (size_t i = 0; i < oldFlattenedNodes.size(); i++) {
586586
auto &oldChild = oldFlattenedNodes[i];
587587

588588
auto unvisitedOtherNodesIt =
@@ -767,7 +767,7 @@ static void calculateShadowViewMutationsV2(
767767
return;
768768
}
769769

770-
auto index = int{0};
770+
size_t index = 0;
771771

772772
// Lists of mutations
773773
auto createMutations = ShadowViewMutation::List{};
@@ -790,7 +790,7 @@ static void calculateShadowViewMutationsV2(
790790
LOG(ERROR) << "Differ Entry: Child Pairs of node: [" << parentShadowView.tag
791791
<< "]";
792792
std::string strOldChildPairs;
793-
for (int oldIndex = 0; oldIndex < oldChildPairs.size(); oldIndex++) {
793+
for (size_t oldIndex = 0; oldIndex < oldChildPairs.size(); oldIndex++) {
794794
strOldChildPairs.append(
795795
std::to_string(oldChildPairs[oldIndex].shadowView.tag));
796796
strOldChildPairs.append(
@@ -799,7 +799,7 @@ static void calculateShadowViewMutationsV2(
799799
strOldChildPairs.append(", ");
800800
}
801801
std::string strNewChildPairs;
802-
for (int newIndex = 0; newIndex < newChildPairs.size(); newIndex++) {
802+
for (size_t newIndex = 0; newIndex < newChildPairs.size(); newIndex++) {
803803
strNewChildPairs.append(
804804
std::to_string(newChildPairs[newIndex].shadowView.tag));
805805
strNewChildPairs.append(
@@ -872,7 +872,7 @@ static void calculateShadowViewMutationsV2(
872872
}
873873
}
874874

875-
int lastIndexAfterFirstStage = index;
875+
size_t lastIndexAfterFirstStage = index;
876876

877877
if (index == newChildPairs.size()) {
878878
// We've reached the end of the new children. We can delete+remove the
@@ -943,9 +943,9 @@ static void calculateShadowViewMutationsV2(
943943
// Walk through both lists at the same time
944944
// We will perform updates, create+insert, remove+delete, remove+insert
945945
// (move) here.
946-
int oldIndex = lastIndexAfterFirstStage,
947-
newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(),
948-
oldSize = oldChildPairs.size();
946+
size_t oldIndex = lastIndexAfterFirstStage,
947+
newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(),
948+
oldSize = oldChildPairs.size();
949949
while (newIndex < newSize || oldIndex < oldSize) {
950950
bool haveNewPair = newIndex < newSize;
951951
bool haveOldPair = oldIndex < oldSize;
@@ -955,8 +955,8 @@ static void calculateShadowViewMutationsV2(
955955
auto const &oldChildPair = oldChildPairs[oldIndex];
956956
auto const &newChildPair = newChildPairs[newIndex];
957957

958-
int newTag = newChildPair.shadowView.tag;
959-
int oldTag = oldChildPair.shadowView.tag;
958+
Tag newTag = newChildPair.shadowView.tag;
959+
Tag oldTag = oldChildPair.shadowView.tag;
960960

961961
if (newTag == oldTag) {
962962
DEBUG_LOGS({
@@ -1050,7 +1050,7 @@ static void calculateShadowViewMutationsV2(
10501050
// children from other nodes, etc.
10511051
auto oldFlattenedNodes = sliceChildShadowNodeViewPairsV2(
10521052
*oldChildPair.shadowNode, true);
1053-
for (int i = 0, j = 0;
1053+
for (size_t i = 0, j = 0;
10541054
i < oldChildPairs.size() && j < oldFlattenedNodes.size();
10551055
i++) {
10561056
auto &oldChild = oldChildPairs[i];
@@ -1119,7 +1119,7 @@ static void calculateShadowViewMutationsV2(
11191119
if (haveOldPair) {
11201120
auto const &oldChildPair = oldChildPairs[oldIndex];
11211121

1122-
int oldTag = oldChildPair.shadowView.tag;
1122+
Tag oldTag = oldChildPair.shadowView.tag;
11231123

11241124
// Was oldTag already inserted? This indicates a reordering, not just
11251125
// a move. The new node has already been inserted, we just need to
@@ -1171,7 +1171,7 @@ static void calculateShadowViewMutationsV2(
11711171
// children from other nodes, etc.
11721172
auto oldFlattenedNodes = sliceChildShadowNodeViewPairsV2(
11731173
*oldChildPair.shadowNode, true);
1174-
for (int i = 0, j = 0;
1174+
for (size_t i = 0, j = 0;
11751175
i < oldChildPairs.size() && j < oldFlattenedNodes.size();
11761176
i++) {
11771177
auto &oldChild = oldChildPairs[i];
@@ -1565,7 +1565,7 @@ static void calculateShadowViewMutations(
15651565
std::move(newGrandChildPairs));
15661566
}
15671567

1568-
int lastIndexAfterFirstStage = index;
1568+
size_t lastIndexAfterFirstStage = index;
15691569

15701570
if (index == newChildPairs.size()) {
15711571
// We've reached the end of the new children. We can delete+remove the
@@ -1630,9 +1630,9 @@ static void calculateShadowViewMutations(
16301630
// Walk through both lists at the same time
16311631
// We will perform updates, create+insert, remove+delete, remove+insert
16321632
// (move) here.
1633-
int oldIndex = lastIndexAfterFirstStage,
1634-
newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(),
1635-
oldSize = oldChildPairs.size();
1633+
size_t oldIndex = lastIndexAfterFirstStage,
1634+
newIndex = lastIndexAfterFirstStage, newSize = newChildPairs.size(),
1635+
oldSize = oldChildPairs.size();
16361636
while (newIndex < newSize || oldIndex < oldSize) {
16371637
bool haveNewPair = newIndex < newSize;
16381638
bool haveOldPair = oldIndex < oldSize;
@@ -1642,8 +1642,8 @@ static void calculateShadowViewMutations(
16421642
auto const &newChildPair = newChildPairs[newIndex];
16431643
auto const &oldChildPair = oldChildPairs[oldIndex];
16441644

1645-
int newTag = newChildPair.shadowView.tag;
1646-
int oldTag = oldChildPair.shadowView.tag;
1645+
Tag newTag = newChildPair.shadowView.tag;
1646+
Tag oldTag = oldChildPair.shadowView.tag;
16471647

16481648
if (newTag == oldTag) {
16491649
DEBUG_LOGS({
@@ -1690,7 +1690,7 @@ static void calculateShadowViewMutations(
16901690

16911691
if (haveOldPair) {
16921692
auto const &oldChildPair = oldChildPairs[oldIndex];
1693-
int oldTag = oldChildPair.shadowView.tag;
1693+
Tag oldTag = oldChildPair.shadowView.tag;
16941694

16951695
// Was oldTag already inserted? This indicates a reordering, not just
16961696
// a move. The new node has already been inserted, we just need to

ReactCommon/react/renderer/mounting/ShadowView.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ struct ShadowViewNodePair final {
7070
bool flattened{false};
7171
bool isConcreteView{true};
7272

73-
int mountIndex{0};
73+
size_t mountIndex{0};
7474

7575
bool inOtherTree{false};
7676

0 commit comments

Comments
 (0)