From eabf29e3203e073e15022301d72b0dc61f3efdc1 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Fri, 22 Jun 2018 07:28:36 -0700 Subject: [PATCH] Fabric: Getting rid of many `auto &&` Summary: @public After reading about move-semantic and rvalue refs I realized that we (I) definitely overuse `auto &&` (aka universal reference) construction. Even if this is harmless, does not look good and idiomatic. Whenever I used that from a semantical point of view I always meant "I need an alias for this" which is actually "read-only reference" which is `const auto &`. This is also fit good to our policy where "everything is const (immutable) by default". Hence I change that to how it should be. Reviewed By: fkgozali Differential Revision: D8475637 fbshipit-source-id: 0a691ededa0e798db8ffa053bff0f400913ab7b8 --- .../RCTActivityIndicatorViewComponentView.mm | 2 +- .../Switch/RCTSwitchComponentView.mm | 2 +- React/Fabric/RCTSurfaceTouchHandler.mm | 12 +++++------ .../ConcreteComponentDescriptor.h | 4 ++-- .../fabric/core/events/EventEmitter.cpp | 4 ++-- ReactCommon/fabric/core/propsConversions.h | 8 ++++---- .../scrollview/ScrollViewShadowNode.cpp | 4 ++-- .../text/basetext/BaseTextShadowNode.cpp | 2 +- ReactCommon/fabric/uimanager/Scheduler.cpp | 18 ++++++++--------- ReactCommon/fabric/uimanager/ShadowTree.cpp | 20 +++++++++---------- .../fabric/view/ConcreteViewShadowNode.h | 2 +- ReactCommon/fabric/view/ViewEventEmitter.cpp | 4 ++-- ReactCommon/fabric/view/ViewProps.cpp | 2 +- ReactCommon/fabric/view/conversions.h | 6 +++--- 14 files changed, 45 insertions(+), 45 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm index b0905c9bd..e275169a1 100644 --- a/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm @@ -30,7 +30,7 @@ static UIActivityIndicatorViewStyle convertActivityIndicatorViewStyle(const Acti _activityIndicatorView = [[UIActivityIndicatorView alloc] initWithFrame:self.bounds]; _activityIndicatorView.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; - auto &&defaultProps = ActivityIndicatorViewProps(); + const auto &defaultProps = ActivityIndicatorViewProps(); if (defaultProps.animating) { [_activityIndicatorView startAnimating]; diff --git a/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm b/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm index ec54418bf..f58ac227c 100644 --- a/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm @@ -26,7 +26,7 @@ using namespace facebook::react; action:@selector(onChange:) forControlEvents:UIControlEventValueChanged]; - auto &&defaultProps = SwitchProps(); + const auto &defaultProps = SwitchProps(); _switchView.on = defaultProps.value; diff --git a/React/Fabric/RCTSurfaceTouchHandler.mm b/React/Fabric/RCTSurfaceTouchHandler.mm index b668da0d8..c26f15bb7 100644 --- a/React/Fabric/RCTSurfaceTouchHandler.mm +++ b/React/Fabric/RCTSurfaceTouchHandler.mm @@ -191,7 +191,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)initWithTarget:(id)target action:(SEL)action - (void)_registerTouches:(NSSet *)touches { for (UITouch *touch in touches) { - auto &&activeTouch = CreateTouchWithUITouch(touch, _rootComponentView); + auto activeTouch = CreateTouchWithUITouch(touch, _rootComponentView); activeTouch.touch.identifier = _identifierPool.dequeue(); _activeTouches.emplace(touch, activeTouch); } @@ -207,7 +207,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)initWithTarget:(id)target action:(SEL)action - (void)_unregisterTouches:(NSSet *)touches { for (UITouch *touch in touches) { - auto &&activeTouch = _activeTouches[touch]; + const auto &activeTouch = _activeTouches[touch]; _identifierPool.enqueue(activeTouch.touch.identifier); _activeTouches.erase(touch); } @@ -221,7 +221,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)initWithTarget:(id)target action:(SEL)action BOOL isEndishEventType = eventType == RCTTouchEventTypeTouchEnd || eventType == RCTTouchEventTypeTouchCancel; for (UITouch *touch in touches) { - auto &&activeTouch = _activeTouches[touch]; + const auto &activeTouch = _activeTouches[touch]; if (!activeTouch.eventEmitter) { continue; @@ -232,7 +232,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)initWithTarget:(id)target action:(SEL)action uniqueEventEmitter.insert(activeTouch.eventEmitter); } - for (auto &&pair : _activeTouches) { + for (const auto &pair : _activeTouches) { if (!pair.second.eventEmitter) { continue; } @@ -247,10 +247,10 @@ RCT_NOT_IMPLEMENTED(- (instancetype)initWithTarget:(id)target action:(SEL)action event.touches.insert(pair.second.touch); } - for (auto &&eventEmitter : uniqueEventEmitter) { + for (const auto &eventEmitter : uniqueEventEmitter) { event.targetTouches.clear(); - for (auto &&pair : _activeTouches) { + for (const auto &pair : _activeTouches) { if (pair.second.eventEmitter == eventEmitter) { event.targetTouches.insert(pair.second.touch); } diff --git a/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h b/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h index 4af3582c1..2f1f7fcd9 100644 --- a/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h +++ b/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h @@ -64,7 +64,7 @@ public: assert(std::dynamic_pointer_cast(props)); assert(std::dynamic_pointer_cast(eventEmitter)); - auto &&shadowNode = std::make_shared( + const auto &shadowNode = std::make_shared( tag, rootTag, std::static_pointer_cast(props), @@ -85,7 +85,7 @@ public: ) const override { assert(std::dynamic_pointer_cast(sourceShadowNode)); - auto &&shadowNode = std::make_shared( + const auto &shadowNode = std::make_shared( std::static_pointer_cast(sourceShadowNode), std::static_pointer_cast(props), std::static_pointer_cast(eventEmitter), diff --git a/ReactCommon/fabric/core/events/EventEmitter.cpp b/ReactCommon/fabric/core/events/EventEmitter.cpp index fdb1b9889..9c61be13f 100644 --- a/ReactCommon/fabric/core/events/EventEmitter.cpp +++ b/ReactCommon/fabric/core/events/EventEmitter.cpp @@ -33,7 +33,7 @@ void EventEmitter::dispatchEvent( const folly::dynamic &payload, const EventPriority &priority ) const { - auto &&eventDispatcher = eventDispatcher_.lock(); + const auto &eventDispatcher = eventDispatcher_.lock(); if (!eventDispatcher) { return; } @@ -50,7 +50,7 @@ void EventEmitter::dispatchEvent( } EventTarget EventEmitter::createEventTarget() const { - auto &&eventDispatcher = eventDispatcher_.lock(); + const auto &eventDispatcher = eventDispatcher_.lock(); assert(eventDispatcher); return eventDispatcher->createEventTarget(instanceHandle_); } diff --git a/ReactCommon/fabric/core/propsConversions.h b/ReactCommon/fabric/core/propsConversions.h index 0c259129d..f255d6c80 100644 --- a/ReactCommon/fabric/core/propsConversions.h +++ b/ReactCommon/fabric/core/propsConversions.h @@ -41,12 +41,12 @@ inline T convertRawProp( const T &sourceValue, const T &defaultValue = T() ) { - auto &&iterator = rawProps.find(name); + const auto &iterator = rawProps.find(name); if (iterator == rawProps.end()) { return sourceValue; } - auto &&value = iterator->second; + const auto &value = iterator->second; // Special case: `null` always means `the prop was removed, use default value`. if (value.isNull()) { @@ -65,12 +65,12 @@ inline static folly::Optional convertRawProp( const folly::Optional &sourceValue, const folly::Optional &defaultValue = {} ) { - auto &&iterator = rawProps.find(name); + const auto &iterator = rawProps.find(name); if (iterator == rawProps.end()) { return sourceValue; } - auto &&value = iterator->second; + const auto &value = iterator->second; // Special case: `null` always means `the prop was removed, use default value`. if (value.isNull()) { diff --git a/ReactCommon/fabric/scrollview/ScrollViewShadowNode.cpp b/ReactCommon/fabric/scrollview/ScrollViewShadowNode.cpp index 2d3efaa89..5662889f2 100644 --- a/ReactCommon/fabric/scrollview/ScrollViewShadowNode.cpp +++ b/ReactCommon/fabric/scrollview/ScrollViewShadowNode.cpp @@ -22,11 +22,11 @@ void ScrollViewShadowNode::updateLocalData() { ensureUnsealed(); Rect contentBoundingRect; - for (auto &&childNode : getLayoutableChildNodes()) { + for (const auto &childNode : getLayoutableChildNodes()) { contentBoundingRect.unionInPlace(childNode->getLayoutMetrics().frame); } - auto &&localData = std::make_shared(contentBoundingRect); + const auto &localData = std::make_shared(contentBoundingRect); setLocalData(localData); } diff --git a/ReactCommon/fabric/text/basetext/BaseTextShadowNode.cpp b/ReactCommon/fabric/text/basetext/BaseTextShadowNode.cpp index d812d242c..61e4c65f3 100644 --- a/ReactCommon/fabric/text/basetext/BaseTextShadowNode.cpp +++ b/ReactCommon/fabric/text/basetext/BaseTextShadowNode.cpp @@ -25,7 +25,7 @@ AttributedString BaseTextShadowNode::getAttributedString( AttributedString attributedString; - for (auto &&childNode : *childNodes) { + for (const auto &childNode : *childNodes) { // RawShadowNode SharedRawTextShadowNode rawTextShadowNode = std::dynamic_pointer_cast(childNode); if (rawTextShadowNode) { diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index 8a5f2f6e2..c7e3f5110 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -24,8 +24,8 @@ namespace react { Scheduler::Scheduler(const SharedContextContainer &contextContainer): contextContainer_(contextContainer) { - auto &&eventDispatcher = std::make_shared(); - auto &&componentDescriptorRegistry = ComponentDescriptorFactory::buildRegistry(eventDispatcher, contextContainer); + const auto &eventDispatcher = std::make_shared(); + const auto &componentDescriptorRegistry = ComponentDescriptorFactory::buildRegistry(eventDispatcher, contextContainer); uiManager_ = std::make_shared(componentDescriptorRegistry); uiManager_->setDelegate(this); @@ -40,27 +40,27 @@ Scheduler::~Scheduler() { } void Scheduler::registerRootTag(Tag rootTag) { - auto &&shadowTree = std::make_shared(rootTag); + const auto &shadowTree = std::make_shared(rootTag); shadowTree->setDelegate(this); shadowTreeRegistry_.insert({rootTag, shadowTree}); } void Scheduler::unregisterRootTag(Tag rootTag) { - auto &&iterator = shadowTreeRegistry_.find(rootTag); - auto &&shadowTree = iterator->second; + const auto &iterator = shadowTreeRegistry_.find(rootTag); + const auto &shadowTree = iterator->second; assert(shadowTree); shadowTree->setDelegate(nullptr); shadowTreeRegistry_.erase(iterator); } Size Scheduler::measure(const Tag &rootTag, const LayoutConstraints &layoutConstraints, const LayoutContext &layoutContext) const { - auto &&shadowTree = shadowTreeRegistry_.at(rootTag); + const auto &shadowTree = shadowTreeRegistry_.at(rootTag); assert(shadowTree); return shadowTree->measure(layoutConstraints, layoutContext); } void Scheduler::constraintLayout(const Tag &rootTag, const LayoutConstraints &layoutConstraints, const LayoutContext &layoutContext) { - auto &&shadowTree = shadowTreeRegistry_.at(rootTag); + const auto &shadowTree = shadowTreeRegistry_.at(rootTag); assert(shadowTree); return shadowTree->constraintLayout(layoutConstraints, layoutContext); } @@ -86,8 +86,8 @@ void Scheduler::shadowTreeDidCommit(const SharedShadowTree &shadowTree, const Tr #pragma mark - UIManagerDelegate void Scheduler::uiManagerDidFinishTransaction(Tag rootTag, const SharedShadowNodeUnsharedList &rootChildNodes) { - auto &&iterator = shadowTreeRegistry_.find(rootTag); - auto &&shadowTree = iterator->second; + const auto &iterator = shadowTreeRegistry_.find(rootTag); + const auto &shadowTree = iterator->second; assert(shadowTree); return shadowTree->complete(rootChildNodes); } diff --git a/ReactCommon/fabric/uimanager/ShadowTree.cpp b/ReactCommon/fabric/uimanager/ShadowTree.cpp index bc19c1b32..55e9b5038 100644 --- a/ReactCommon/fabric/uimanager/ShadowTree.cpp +++ b/ReactCommon/fabric/uimanager/ShadowTree.cpp @@ -18,7 +18,7 @@ namespace react { ShadowTree::ShadowTree(Tag rootTag): rootTag_(rootTag) { - auto &&noopEventEmitter = std::make_shared(nullptr, rootTag, nullptr); + const auto &noopEventEmitter = std::make_shared(nullptr, rootTag, nullptr); rootShadowNode_ = std::make_shared( rootTag, rootTag, @@ -50,7 +50,7 @@ void ShadowTree::constraintLayout(const LayoutConstraints &layoutConstraints, co UnsharedRootShadowNode ShadowTree::cloneRootShadowNode(const LayoutConstraints &layoutConstraints, const LayoutContext &layoutContext) const { auto oldRootShadowNode = rootShadowNode_; - auto &&props = std::make_shared(*oldRootShadowNode->getProps(), layoutConstraints, layoutContext); + const auto &props = std::make_shared(*oldRootShadowNode->getProps(), layoutConstraints, layoutContext); auto newRootShadowNode = std::make_shared(oldRootShadowNode, props, nullptr, nullptr); return newRootShadowNode; } @@ -99,32 +99,32 @@ bool ShadowTree::commit(const SharedRootShadowNode &newRootShadowNode) { } void ShadowTree::emitLayoutEvents(const TreeMutationInstructionList &instructions) { - for (auto &&instruction : instructions) { - auto &&type = instruction.getType(); + for (const auto &instruction : instructions) { + const auto &type = instruction.getType(); // Only `Insertion` and `Replacement` instructions can affect layout metrics. if ( type == TreeMutationInstruction::Insertion || type == TreeMutationInstruction::Replacement ) { - auto &&newShadowNode = instruction.getNewChildNode(); - auto &&eventEmitter = newShadowNode->getEventEmitter(); - auto &&viewEventEmitter = std::dynamic_pointer_cast(eventEmitter); + const auto &newShadowNode = instruction.getNewChildNode(); + const auto &eventEmitter = newShadowNode->getEventEmitter(); + const auto &viewEventEmitter = std::dynamic_pointer_cast(eventEmitter); // Checking if particular shadow node supports `onLayout` event (part of `ViewEventEmitter`). if (viewEventEmitter) { // Now we know that both (old and new) shadow nodes must be `LayoutableShadowNode` subclasses. assert(std::dynamic_pointer_cast(newShadowNode)); // TODO(T29661055): Consider using `std::reinterpret_pointer_cast`. - auto &&newLayoutableShadowNode = + const auto &newLayoutableShadowNode = std::dynamic_pointer_cast(newShadowNode); // In case if we have `oldShadowNode`, we have to check that layout metrics have changed. if (type == TreeMutationInstruction::Replacement) { - auto &&oldShadowNode = instruction.getOldChildNode(); + const auto &oldShadowNode = instruction.getOldChildNode(); assert(std::dynamic_pointer_cast(oldShadowNode)); // TODO(T29661055): Consider using `std::reinterpret_pointer_cast`. - auto &&oldLayoutableShadowNode = + const auto &oldLayoutableShadowNode = std::dynamic_pointer_cast(oldShadowNode); if (oldLayoutableShadowNode->getLayoutMetrics() == newLayoutableShadowNode->getLayoutMetrics()) { diff --git a/ReactCommon/fabric/view/ConcreteViewShadowNode.h b/ReactCommon/fabric/view/ConcreteViewShadowNode.h index dc898dcdd..c65f9ea06 100644 --- a/ReactCommon/fabric/view/ConcreteViewShadowNode.h +++ b/ReactCommon/fabric/view/ConcreteViewShadowNode.h @@ -131,7 +131,7 @@ public: return false; } - auto &&other = static_cast(rhs); + const auto &other = static_cast(rhs); return getLayoutMetrics() == other.getLayoutMetrics(); } diff --git a/ReactCommon/fabric/view/ViewEventEmitter.cpp b/ReactCommon/fabric/view/ViewEventEmitter.cpp index fe1b246db..643b0b017 100644 --- a/ReactCommon/fabric/view/ViewEventEmitter.cpp +++ b/ReactCommon/fabric/view/ViewEventEmitter.cpp @@ -28,7 +28,7 @@ void ViewEventEmitter::onAccessibilityMagicTap() const { void ViewEventEmitter::onLayout(const LayoutMetrics &layoutMetrics) const { folly::dynamic payload = folly::dynamic::object(); - auto &&frame = layoutMetrics.frame; + const auto &frame = layoutMetrics.frame; payload["layout"] = folly::dynamic::object ("x", frame.origin.x) ("y", frame.origin.y) @@ -57,7 +57,7 @@ static folly::dynamic touchPayload(const Touch &touch) { static folly::dynamic touchesPayload(const Touches &touches) { folly::dynamic array = folly::dynamic::array(); - for (auto &&touch : touches) { + for (const auto &touch : touches) { array.push_back(touchPayload(touch)); } return array; diff --git a/ReactCommon/fabric/view/ViewProps.cpp b/ReactCommon/fabric/view/ViewProps.cpp index b6d37f5f0..bdccfedc4 100644 --- a/ReactCommon/fabric/view/ViewProps.cpp +++ b/ReactCommon/fabric/view/ViewProps.cpp @@ -42,7 +42,7 @@ ViewProps::ViewProps(const ViewProps &sourceProps, const RawProps &rawProps): #pragma mark - DebugStringConvertible SharedDebugStringConvertibleList ViewProps::getDebugProps() const { - auto &&defaultViewProps = ViewProps(); + const auto &defaultViewProps = ViewProps(); return AccessibilityProps::getDebugProps() + diff --git a/ReactCommon/fabric/view/conversions.h b/ReactCommon/fabric/view/conversions.h index e3fd02ead..64f1478d4 100644 --- a/ReactCommon/fabric/view/conversions.h +++ b/ReactCommon/fabric/view/conversions.h @@ -230,11 +230,11 @@ inline void fromDynamic(const folly::dynamic &value, YGFloatOptional &result) { inline void fromDynamic(const folly::dynamic &value, Transform &result) { assert(value.isArray()); Transform transformMatrix; - for (auto &&tranformConfiguration : value) { + for (const auto &tranformConfiguration : value) { assert(tranformConfiguration.isObject()); auto pair = *tranformConfiguration.items().begin(); - auto &&operation = pair.first.asString(); - auto &¶meters = pair.second; + const auto &operation = pair.first.asString(); + const auto ¶meters = pair.second; if (operation == "matrix") { assert(parameters.isArray());