From 4157a49d8d8fbad1f12b4d24391e1c5f5bbfd534 Mon Sep 17 00:00:00 2001 From: David Aurelio Date: Thu, 6 Dec 2018 07:35:10 -0800 Subject: [PATCH] Eliminate `YGFloatOptional::getValue()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: @public `YGFloatOptional::getValue()` has the unfortunate property of calling `std::exit` if the wrapped value is undefined. That forces `x.isUndefined() ? fallback : x.getValue()` as access pattern. Here, we replace that by introducing `YGFloatOptional::orElse(float)` which encapsulates that pattern. Other additions are `orElseGet([] { … })` and some extra operators. Reviewed By: SidharthGuglani Differential Revision: D13209152 fbshipit-source-id: 4e5deceaaaaf8eaed44846a8c152cc8b235e815c --- .../fabric/components/view/conversions.h | 4 +- ReactCommon/yoga/yoga/Utils.cpp | 9 ++- ReactCommon/yoga/yoga/YGFloatOptional.cpp | 17 ++-- ReactCommon/yoga/yoga/YGFloatOptional.h | 27 +++++-- ReactCommon/yoga/yoga/YGNode.cpp | 47 +++++------ ReactCommon/yoga/yoga/YGNodePrint.cpp | 3 +- ReactCommon/yoga/yoga/YGStyle.cpp | 37 ++------- ReactCommon/yoga/yoga/Yoga.cpp | 79 ++++++++----------- 8 files changed, 94 insertions(+), 129 deletions(-) diff --git a/ReactCommon/fabric/components/view/conversions.h b/ReactCommon/fabric/components/view/conversions.h index d4274e4ded4..2edafb32223 100644 --- a/ReactCommon/fabric/components/view/conversions.h +++ b/ReactCommon/fabric/components/view/conversions.h @@ -40,7 +40,7 @@ inline Float floatFromYogaOptionalFloat(YGFloatOptional value) { return kFloatUndefined; } - return floatFromYogaFloat(value.getValue()); + return floatFromYogaFloat(value.unwrap()); } inline YGFloatOptional yogaOptionalFloatFromFloat(Float value) { @@ -580,7 +580,7 @@ inline std::string toString(const YGFloatOptional &value) { return "undefined"; } - return folly::to(floatFromYogaFloat(value.getValue())); + return folly::to(floatFromYogaFloat(value.unwrap())); } inline std::string toString( diff --git a/ReactCommon/yoga/yoga/Utils.cpp b/ReactCommon/yoga/yoga/Utils.cpp index 74cc38c9905..1d8eb71e877 100644 --- a/ReactCommon/yoga/yoga/Utils.cpp +++ b/ReactCommon/yoga/yoga/Utils.cpp @@ -56,14 +56,17 @@ float YGFloatSanitize(const float val) { } float YGUnwrapFloatOptional(const YGFloatOptional& op) { - return op.isUndefined() ? YGUndefined : op.getValue(); + return op.unwrap(); } YGFloatOptional YGFloatOptionalMax( const YGFloatOptional& op1, const YGFloatOptional& op2) { - if (!op1.isUndefined() && !op2.isUndefined()) { - return op1.getValue() > op2.getValue() ? op1 : op2; + if (op1 > op2) { + return op1; + } + if (op2 > op1) { + return op2; } return op1.isUndefined() ? op2 : op1; } diff --git a/ReactCommon/yoga/yoga/YGFloatOptional.cpp b/ReactCommon/yoga/yoga/YGFloatOptional.cpp index 0bf89f29b7c..263f24cbd46 100644 --- a/ReactCommon/yoga/yoga/YGFloatOptional.cpp +++ b/ReactCommon/yoga/yoga/YGFloatOptional.cpp @@ -12,15 +12,6 @@ using namespace facebook; -float YGFloatOptional::getValue() const { - if (isUndefined()) { - // Abort, accessing a value of an undefined float optional - std::cerr << "Tried to get value of an undefined YGFloatOptional\n"; - std::exit(EXIT_FAILURE); - } - return value_; -} - bool YGFloatOptional::operator==(YGFloatOptional op) const { return value_ == op.value_ || (isUndefined() && op.isUndefined()); } @@ -37,10 +28,18 @@ bool YGFloatOptional::operator!=(float val) const { return !(*this == val); } +YGFloatOptional YGFloatOptional::operator-() const { + return YGFloatOptional{-value_}; +} + YGFloatOptional YGFloatOptional::operator+(YGFloatOptional op) const { return YGFloatOptional{value_ + op.value_}; } +YGFloatOptional YGFloatOptional::operator-(YGFloatOptional op) const { + return YGFloatOptional{value_ - op.value_}; +} + bool YGFloatOptional::operator>(YGFloatOptional op) const { return value_ > op.value_; } diff --git a/ReactCommon/yoga/yoga/YGFloatOptional.h b/ReactCommon/yoga/yoga/YGFloatOptional.h index 7b573d38860..f6e654fbc89 100644 --- a/ReactCommon/yoga/yoga/YGFloatOptional.h +++ b/ReactCommon/yoga/yoga/YGFloatOptional.h @@ -6,7 +6,6 @@ */ #pragma once -#include #include struct YGFloatOptional { @@ -17,16 +16,28 @@ struct YGFloatOptional { explicit constexpr YGFloatOptional(float value) : value_(value) {} constexpr YGFloatOptional() = default; - // Program will terminate if the value of an undefined is accessed. Please - // make sure to check if the optional is defined before calling this function. - // To check if float optional is defined, use `isUndefined()`. - float getValue() const; - - bool isUndefined() const { - return std::isnan(value_); + // returns the wrapped value, or a value x with YGIsUndefined(x) == true + float unwrap() const { + return value_; } + constexpr bool isUndefined() const { + // std::isnan is not constexpr + return !(value_ == value_); + } + + constexpr float orElse(float other) const { + return isUndefined() ? other : value_; + } + + template + constexpr float orElseGet(Factory&& f) const { + return isUndefined() ? f() : value_; + } + + YGFloatOptional operator-() const; YGFloatOptional operator+(YGFloatOptional op) const; + YGFloatOptional operator-(YGFloatOptional op) const; bool operator>(YGFloatOptional op) const; bool operator<(YGFloatOptional op) const; bool operator>=(YGFloatOptional op) const; diff --git a/ReactCommon/yoga/yoga/YGNode.cpp b/ReactCommon/yoga/yoga/YGNode.cpp index 04c3c7f6dea..558a893739f 100644 --- a/ReactCommon/yoga/yoga/YGNode.cpp +++ b/ReactCommon/yoga/yoga/YGNode.cpp @@ -209,11 +209,7 @@ YGFloatOptional YGNode::relativePosition( return getLeadingPosition(axis, axisSize); } - YGFloatOptional trailingPosition = getTrailingPosition(axis, axisSize); - if (!trailingPosition.isUndefined()) { - trailingPosition = YGFloatOptional{-1 * trailingPosition.getValue()}; - } - return trailingPosition; + return -getTrailingPosition(axis, axisSize); } void YGNode::setPosition( @@ -304,7 +300,7 @@ YGValue YGNode::resolveFlexBasisPtr() const { if (flexBasis.unit != YGUnitAuto && flexBasis.unit != YGUnitUndefined) { return flexBasis; } - if (!style_.flex.isUndefined() && style_.flex.getValue() > 0.0f) { + if (style_.flex > YGFloatOptional{0.0f}) { return config_->useWebDefaults ? YGValueAuto : YGValueZero; } return YGValueAuto; @@ -394,27 +390,23 @@ float YGNode::resolveFlexGrow() { if (owner_ == nullptr) { return 0.0; } - if (!style_.flexGrow.isUndefined()) { - return style_.flexGrow.getValue(); - } - if (!style_.flex.isUndefined() && style_.flex.getValue() > 0.0f) { - return style_.flex.getValue(); - } - return kDefaultFlexGrow; + + return style_.flexGrow.orElseGet( + [this] { return style_.flex.orElse(kDefaultFlexGrow); }); } float YGNode::resolveFlexShrink() { if (owner_ == nullptr) { return 0.0; } - if (!style_.flexShrink.isUndefined()) { - return style_.flexShrink.getValue(); - } - if (!config_->useWebDefaults && !style_.flex.isUndefined() && - style_.flex.getValue() < 0.0f) { - return -style_.flex.getValue(); - } - return config_->useWebDefaults ? kWebDefaultFlexShrink : kDefaultFlexShrink; + return style_.flexShrink.orElseGet([this] { + if (style_.flex < YGFloatOptional{0.0f} && !config_->useWebDefaults) { + return -style_.flex.unwrap(); + } else { + return config_->useWebDefaults ? kWebDefaultFlexShrink + : kDefaultFlexShrink; + } + }); } bool YGNode::isNodeFlexible() { @@ -455,9 +447,7 @@ YGFloatOptional YGNode::getLeadingPadding( const float widthSize) const { const YGFloatOptional& paddingEdgeStart = YGResolveValue(style_.padding[YGEdgeStart], widthSize); - if (YGFlexDirectionIsRow(axis) && - style_.padding[YGEdgeStart].unit != YGUnitUndefined && - !paddingEdgeStart.isUndefined() && paddingEdgeStart.getValue() >= 0.0f) { + if (YGFlexDirectionIsRow(axis) && paddingEdgeStart >= YGFloatOptional{0.0f}) { return paddingEdgeStart; } @@ -470,11 +460,10 @@ YGFloatOptional YGNode::getLeadingPadding( YGFloatOptional YGNode::getTrailingPadding( const YGFlexDirection axis, const float widthSize) const { - if (YGFlexDirectionIsRow(axis) && - style_.padding[YGEdgeEnd].unit != YGUnitUndefined && - !YGResolveValue(style_.padding[YGEdgeEnd], widthSize).isUndefined() && - YGResolveValue(style_.padding[YGEdgeEnd], widthSize).getValue() >= 0.0f) { - return YGResolveValue(style_.padding[YGEdgeEnd], widthSize); + const YGFloatOptional& paddingEdgeEnd = + YGResolveValue(style_.padding[YGEdgeEnd], widthSize); + if (YGFlexDirectionIsRow(axis) && paddingEdgeEnd >= YGFloatOptional{0.0f}) { + return paddingEdgeEnd; } YGFloatOptional resolvedValue = YGResolveValue( diff --git a/ReactCommon/yoga/yoga/YGNodePrint.cpp b/ReactCommon/yoga/yoga/YGNodePrint.cpp index 541a6fef946..c497996e637 100644 --- a/ReactCommon/yoga/yoga/YGNodePrint.cpp +++ b/ReactCommon/yoga/yoga/YGNodePrint.cpp @@ -43,7 +43,7 @@ static void appendFloatOptionalIfDefined( const string key, const YGFloatOptional num) { if (!num.isUndefined()) { - appendFormatedString(base, "%s: %g; ", key.c_str(), num.getValue()); + appendFormatedString(base, "%s: %g; ", key.c_str(), num.unwrap()); } } @@ -71,7 +71,6 @@ appendNumberIfNotAuto(string* base, const string& key, const YGValue number) { static void appendNumberIfNotZero(string* base, const string& str, const YGValue number) { - if (number.unit == YGUnitAuto) { base->append(str + ": auto; "); } else if (!YGFloatsEqual(number.value, 0)) { diff --git a/ReactCommon/yoga/yoga/YGStyle.cpp b/ReactCommon/yoga/yoga/YGStyle.cpp index bc90463e739..a2f4a17e967 100644 --- a/ReactCommon/yoga/yoga/YGStyle.cpp +++ b/ReactCommon/yoga/yoga/YGStyle.cpp @@ -8,8 +8,8 @@ // Yoga specific properties, not compatible with flexbox specification bool YGStyle::operator==(const YGStyle& style) { - bool areNonFloatValuesEqual = direction == style.direction && - flexDirection == style.flexDirection && + return ( + direction == style.direction && flexDirection == style.flexDirection && justifyContent == style.justifyContent && alignContent == style.alignContent && alignItems == style.alignItems && alignSelf == style.alignSelf && positionType == style.positionType && @@ -21,34 +21,7 @@ bool YGStyle::operator==(const YGStyle& style) { YGValueArrayEqual(border, style.border) && YGValueArrayEqual(dimensions, style.dimensions) && YGValueArrayEqual(minDimensions, style.minDimensions) && - YGValueArrayEqual(maxDimensions, style.maxDimensions); - - areNonFloatValuesEqual = - areNonFloatValuesEqual && flex.isUndefined() == style.flex.isUndefined(); - if (areNonFloatValuesEqual && !flex.isUndefined() && - !style.flex.isUndefined()) { - areNonFloatValuesEqual = - areNonFloatValuesEqual && flex.getValue() == style.flex.getValue(); - } - - areNonFloatValuesEqual = areNonFloatValuesEqual && - flexGrow.isUndefined() == style.flexGrow.isUndefined(); - if (areNonFloatValuesEqual && !flexGrow.isUndefined()) { - areNonFloatValuesEqual = areNonFloatValuesEqual && - flexGrow.getValue() == style.flexGrow.getValue(); - } - - areNonFloatValuesEqual = areNonFloatValuesEqual && - flexShrink.isUndefined() == style.flexShrink.isUndefined(); - if (areNonFloatValuesEqual && !style.flexShrink.isUndefined()) { - areNonFloatValuesEqual = areNonFloatValuesEqual && - flexShrink.getValue() == style.flexShrink.getValue(); - } - - if (!(aspectRatio.isUndefined() && style.aspectRatio.isUndefined())) { - areNonFloatValuesEqual = areNonFloatValuesEqual && - aspectRatio.getValue() == style.aspectRatio.getValue(); - } - - return areNonFloatValuesEqual; + YGValueArrayEqual(maxDimensions, style.maxDimensions) && + flex == style.flex && flexGrow == style.flexGrow && + flexShrink == style.flexShrink && aspectRatio == style.aspectRatio); } diff --git a/ReactCommon/yoga/yoga/Yoga.cpp b/ReactCommon/yoga/yoga/Yoga.cpp index 246e0f41248..5197d994800 100644 --- a/ReactCommon/yoga/yoga/Yoga.cpp +++ b/ReactCommon/yoga/yoga/Yoga.cpp @@ -579,16 +579,14 @@ void YGNodeCopyStyle(const YGNodeRef dstNode, const YGNodeRef srcNode) { } float YGNodeStyleGetFlexGrow(const YGNodeRef node) { - return node->getStyle().flexGrow.isUndefined() - ? kDefaultFlexGrow - : node->getStyle().flexGrow.getValue(); + return node->getStyle().flexGrow.orElse(kDefaultFlexGrow); } float YGNodeStyleGetFlexShrink(const YGNodeRef node) { - return node->getStyle().flexShrink.isUndefined() - ? (node->getConfig()->useWebDefaults ? kWebDefaultFlexShrink - : kDefaultFlexShrink) - : node->getStyle().flexShrink.getValue(); + return node->getStyle().flexShrink.orElseGet([node] { + return node->getConfig()->useWebDefaults ? kWebDefaultFlexShrink + : kDefaultFlexShrink; + }); } namespace { @@ -856,8 +854,7 @@ void YGNodeStyleSetFlex(const YGNodeRef node, const float flex) { // TODO(T26792433): Change the API to accept YGFloatOptional. float YGNodeStyleGetFlex(const YGNodeRef node) { - return node->getStyle().flex.isUndefined() ? YGUndefined - : node->getStyle().flex.getValue(); + return node->getStyle().flex.orElse(YGUndefined); } // TODO(T26792433): Change the API to accept YGFloatOptional. @@ -959,7 +956,7 @@ float YGNodeStyleGetBorder(const YGNodeRef node, const YGEdge edge) { // TODO(T26792433): Change the API to accept YGFloatOptional. float YGNodeStyleGetAspectRatio(const YGNodeRef node) { const YGFloatOptional op = node->getStyle().aspectRatio; - return op.isUndefined() ? YGUndefined : op.getValue(); + return op.orElse(YGUndefined); } // TODO(T26792433): Change the API to accept YGFloatOptional. @@ -1229,15 +1226,15 @@ static YGFloatOptional YGNodeBoundAxisWithinMinAndMax( node->getStyle().maxDimensions[YGDimensionWidth], axisSize); } - if (!max.isUndefined() && max.getValue() >= 0 && value > max.getValue()) { + if (max >= YGFloatOptional{0} && YGFloatOptional{value} > max) { return max; } - if (!min.isUndefined() && min.getValue() >= 0 && value < min.getValue()) { + if (min >= YGFloatOptional{0} && YGFloatOptional{value} < min) { return min; } - return YGFloatOptional(value); + return YGFloatOptional{value}; } // Like YGNodeBoundAxisWithinMinAndMax but also ensures that the value doesn't @@ -1278,14 +1275,14 @@ static void YGConstrainMaxSizeForMode( switch (*mode) { case YGMeasureModeExactly: case YGMeasureModeAtMost: - *size = (maxSize.isUndefined() || *size < maxSize.getValue()) - ? *size - : maxSize.getValue(); + if (YGFloatOptional{*size} > maxSize) { + *size = maxSize.unwrap(); + } break; case YGMeasureModeUndefined: if (!maxSize.isUndefined()) { *mode = YGMeasureModeAtMost; - *size = maxSize.getValue(); + *size = maxSize.unwrap(); } break; } @@ -1395,16 +1392,15 @@ static void YGNodeComputeFlexBasisForChild( } } - if (!child->getStyle().aspectRatio.isUndefined()) { + auto hasAspectRatio = !child->getStyle().aspectRatio.isUndefined(); + auto aspectRatio = child->getStyle().aspectRatio.unwrap(); + if (hasAspectRatio) { if (!isMainAxisRow && childWidthMeasureMode == YGMeasureModeExactly) { - childHeight = marginColumn + - (childWidth - marginRow) / child->getStyle().aspectRatio.getValue(); + childHeight = marginColumn + (childWidth - marginRow) / aspectRatio; childHeightMeasureMode = YGMeasureModeExactly; } else if ( isMainAxisRow && childHeightMeasureMode == YGMeasureModeExactly) { - childWidth = marginRow + - (childHeight - marginColumn) * - child->getStyle().aspectRatio.getValue(); + childWidth = marginRow + (childHeight - marginColumn) * aspectRatio; childWidthMeasureMode = YGMeasureModeExactly; } } @@ -1422,9 +1418,8 @@ static void YGNodeComputeFlexBasisForChild( childWidthStretch) { childWidth = width; childWidthMeasureMode = YGMeasureModeExactly; - if (!child->getStyle().aspectRatio.isUndefined()) { - childHeight = - (childWidth - marginRow) / child->getStyle().aspectRatio.getValue(); + if (hasAspectRatio) { + childHeight = (childWidth - marginRow) / aspectRatio; childHeightMeasureMode = YGMeasureModeExactly; } } @@ -1439,9 +1434,8 @@ static void YGNodeComputeFlexBasisForChild( childHeight = height; childHeightMeasureMode = YGMeasureModeExactly; - if (!child->getStyle().aspectRatio.isUndefined()) { - childWidth = (childHeight - marginColumn) * - child->getStyle().aspectRatio.getValue(); + if (hasAspectRatio) { + childWidth = (childHeight - marginColumn) * aspectRatio; childWidthMeasureMode = YGMeasureModeExactly; } } @@ -1553,13 +1547,11 @@ static void YGNodeAbsoluteLayoutChild( // flexible. if (YGFloatIsUndefined(childWidth) ^ YGFloatIsUndefined(childHeight)) { if (!child->getStyle().aspectRatio.isUndefined()) { + auto aspectRatio = child->getStyle().aspectRatio.unwrap(); if (YGFloatIsUndefined(childWidth)) { - childWidth = marginRow + - (childHeight - marginColumn) * - child->getStyle().aspectRatio.getValue(); + childWidth = marginRow + (childHeight - marginColumn) * aspectRatio; } else if (YGFloatIsUndefined(childHeight)) { - childHeight = marginColumn + - (childWidth - marginRow) / child->getStyle().aspectRatio.getValue(); + childHeight = marginColumn + (childWidth - marginRow) / aspectRatio; } } } @@ -1885,16 +1877,15 @@ static float YGNodeCalculateAvailableInnerDim( // constraints const YGFloatOptional minDimensionOptional = YGResolveValue(node->getStyle().minDimensions[dimension], ownerDim); - const float minInnerDim = minDimensionOptional.isUndefined() - ? 0.0f - : minDimensionOptional.getValue() - paddingAndBorder; + const float minInnerDim = + (minDimensionOptional - YGFloatOptional{paddingAndBorder}).orElse(0.0f); const YGFloatOptional maxDimensionOptional = YGResolveValue(node->getStyle().maxDimensions[dimension], ownerDim); - const float maxInnerDim = maxDimensionOptional.isUndefined() - ? FLT_MAX - : maxDimensionOptional.getValue() - paddingAndBorder; + const float maxInnerDim = + (maxDimensionOptional - YGFloatOptional{paddingAndBorder}) + .orElse(FLT_MAX); availableInnerDim = YGFloatMax(YGFloatMin(availableInnerDim, maxInnerDim), minInnerDim); } @@ -2162,9 +2153,9 @@ static float YGDistributeFreeSpaceSecondPass( if (!currentRelativeChild->getStyle().aspectRatio.isUndefined()) { childCrossSize = isMainAxisRow ? (childMainSize - marginMain) / - currentRelativeChild->getStyle().aspectRatio.getValue() + currentRelativeChild->getStyle().aspectRatio.unwrap() : (childMainSize - marginMain) * - currentRelativeChild->getStyle().aspectRatio.getValue(); + currentRelativeChild->getStyle().aspectRatio.unwrap(); childCrossMeasureMode = YGMeasureModeExactly; childCrossSize += marginCross; @@ -3131,9 +3122,9 @@ static void YGNodelayoutImpl( ? ((YGUnwrapFloatOptional(child->getMarginForAxis( crossAxis, availableInnerWidth)) + (isMainAxisRow ? childMainSize / - child->getStyle().aspectRatio.getValue() + child->getStyle().aspectRatio.unwrap() : childMainSize * - child->getStyle().aspectRatio.getValue()))) + child->getStyle().aspectRatio.unwrap()))) : collectedFlexItemsValues.crossDim; childMainSize += YGUnwrapFloatOptional(