Summary:
changelog: [internal]
You can read more about this rule on https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
# Isn't it wasteful to copy? Isn't reference more efficient?
This rule of thumb is no longer true since C++11 with move semantics. Let's look at some examples.
# Option one
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
private:
std::string text_;
};
```
By using reference here, we prevent the caller from using rvalue to and avoiding copy. Regardless of what the caller passes in, copy always happens.
# Option two
```
class TextHolder
{
public:
TextBox(std::string const &text) : text_(text) {}
TextBox(std::string &&text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, we provide two constructors, one for const reference and one for rvalue reference. This gives the caller option to avoid copy. But now we have two constructors, which is not ideal.
# Option three (what we do in this diff)
```
class TextHolder
{
public:
TextBox(std::string text) : text_(std::move(text)) {}
private:
std::string text_;
};
```
Here, the caller has option to avoid copy and we only have single constructor.
Reviewed By: fkgozali, JoshuaGross
Differential Revision: D33276841
fbshipit-source-id: 619d5123d2e28937b22874650366629f24f20a63
Summary:
changelog: [internal]
In this diff, we delete default initialised for ShadowViewMutation to prevent accidentally creating empty ShadowViewMutation.
The other initialiser is made private and all of its uses are migrated to designated initialisers. This makes for safer API.
Reviewed By: RSNara
Differential Revision: D30774900
fbshipit-source-id: d2064bf08409850e75e13ad06558b7980a7f5d8d
Summary:
On Android we have the notion of "virtual views", which are defined consistently but the logic is scattered and duplicated throughout the codebase.
The logic exists to mark nodes that exist in the ShadowTree, but not the View tree. We want to CREATE, UPDATE, and DELETE them on the platform, but not INSERT or REMOVE
them. They basically exist as EventEmitter objects.
The only issue with this is (1) duplicated code, which opened the possibility for inconsistent definition (2) StubViewTree did not account for virtualized views, which caused
assert crashes in debug mode for certain LayoutAnimations on Android.
By moving the definition to ShadowViewMutation and accounting for it in StubViewTree, asserts are correct and consistent on all platforms.
This was not caught until recently, because, until recently, no asserts actually ran on Android.
Changelog: [Internal]
Reviewed By: sammy-SC
Differential Revision: D27001199
fbshipit-source-id: eb29085317037ba8a286d7813bdd57095ad4746f
Summary:
Original commit changeset: 3ed8e78e31b0
Backing-out D25938851 (https://github.com/facebook/react-native/commit/69b3016171bb2f994dd4a62c34c2c4645b5a7d56) and D25935785 (https://github.com/facebook/react-native/commit/bdea479a1faa0f1f7d7c9d9162212cce94bc9720). Based on analysis documented in T83141606, I believe this issue should be fixed in JS.
Additionally, this crash actually has nothing to do with (un)flattening or the differ; it is a side-effect of stale ShadowNodes being cloned, which I believe is either UB or a contract violation. Either way, it should probably be fixed either in JS, or in node cloning. So this isn't the right solution for this issue and should be reverted.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25949569
fbshipit-source-id: 8cf1094a767da98fff4430da60d223412e029545
Summary:
Android has some optimizations around view allocation and pre-allocation that, in the case of View Unflattening, can cause "Create" mutations to be skipped.
To make sure that doesn't happen, we add a flag to ShadowViewMutation (in the core) that any platform can consume, that indicates if the mutation is a "recreation" mutation.
It is still a bit unclear why this is needed, in the sense that I would expect props revision to increment if a view is unflattened. However, there is at least one documented reproduction where that is *not* the case. So for now, we'll have a hack pending further investigation.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D25935785
fbshipit-source-id: 6fb4f0a6dedba0fe46ba3cd558ac1daa70f671f5
Summary:
The `index` parameter for UpdateMutation is optional, and is normally just -1. It's not useful, so remove it. `parentShadowView` is also not relevant and is not used; in some existing use-cases the actual parent view of the updated view is available, and in some contexts the parent view is not set.
The function now will always set the index to -1 for UpdateMutations, and `{}` for ParentShadowView.
This should have no impact on iOS or Android, as this parameter is not used. It could theoretically have an impact on lifetimes of objects retained (now not retained) by not passing parentShadowView into the mutation. For example, any shared props or state associated with the parent will not be retained in the Update mutation now.
Changelog: [Internal]
Reviewed By: shergin
Differential Revision: D25342943
fbshipit-source-id: 0ddbef76a6e2eefc2629c9729f721d8674d7737e
Summary:
This diff moves fabric C++ code from ReactCommon/fabric to ReactCommon/react/renderer
As part of this diff I also refactored components, codegen and callsites on CatalystApp, FB4A and venice
Script: P137350694
changelog: [internal] internal refactor
Reviewed By: fkgozali
Differential Revision: D22852139
fbshipit-source-id: f85310ba858b6afd81abfd9cbe6d70b28eca7415
Samuel Susla
Andres Suarez
Joshua Gross
David Vacca