Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1e101369 authored by Lloyd Pique's avatar Lloyd Pique
Browse files

libui: Fix clamping of input values in ui::Size 

A previous change to fix a compiler warning broke the clamped-conversion
of floating point input values to the int32_t values used internally.
As a result, calling setWidth() or steHeight() with any floating point
value resulted in a value of INT32_MIN being set,

The existing Size_test unit test would have caught this issue, but it
would have had to have been manually run since it was not configured to
run automatically. No one noticed it was broken for a while since it is
relatively new and the existing callers were not setting a floating
point value.

This change:

1) Fixes clamping of floating point values to work again, as verified
   by the existing test.

2) Helps enforce that the header remains conversion-warning free by
   enabling a few conversion warnings (as errors) when building the
   test.

3) Adds a TEST_MAPPING file, which will run Size_test as part of the
   presubmit runs (effectively continuously), as well as including the
   test in the device-test suite in the Android.bp file.

4) Adds a PrintTo() function for more clearly printing out a ui::Size
   value when using the Google Test framework.

Bug: 149495759
Test: atest Size_test
Change-Id: I8f18f20e6b5a3ea54eb383486a1f7222a1a2a544
parent 3895bbf1

libs/ui/include/ui/Size.h

+19 −11
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ 
#include <algorithm>

#include <cstdint>

#include <limits>

#include <ostream>

#include <type_traits>

#include <utility>
@@ -113,13 +114,12 @@ struct Size { 
                    std::numeric_limits<Size::remove_cv_reference_t<ToType>>::is_bounded &&

                            std::numeric_limits<Size::remove_cv_reference_t<FromType>>::is_bounded,

                    FromType&&>::type v) {

        static constexpr auto toHighest = std::numeric_limits<remove_cv_reference_t<ToType>>::max();

        static constexpr auto toLowest =

                std::numeric_limits<remove_cv_reference_t<ToType>>::lowest();

        static constexpr auto fromHighest =

                std::numeric_limits<remove_cv_reference_t<FromType>>::max();

        static constexpr auto fromLowest =

                std::numeric_limits<remove_cv_reference_t<FromType>>::lowest();

        using BareToType = remove_cv_reference_t<ToType>;

        using BareFromType = remove_cv_reference_t<FromType>;

        static constexpr auto toHighest = std::numeric_limits<BareToType>::max();

        static constexpr auto toLowest = std::numeric_limits<BareToType>::lowest();

        static constexpr auto fromHighest = std::numeric_limits<BareFromType>::max();

        static constexpr auto fromLowest = std::numeric_limits<BareFromType>::lowest();



        // A clamp is needed if the range of FromType is not a subset of the range of ToType

        static constexpr bool isClampNeeded = (toLowest > fromLowest) || (toHighest < fromHighest);
@@ -129,10 +129,13 @@ struct Size { 
            return static_cast<ToType>(v);

        }



        // Otherwise we leverage implicit conversion to safely compare values of

        // different types, to ensure we return a value clamped to the range of

        // ToType.

        return v < toLowest ? toLowest : (static_cast<ToType>(v) > toHighest ? toHighest : static_cast<ToType>(v));

        // Otherwise we need to carefully compare the limits of ToType (casted

        // for the comparisons to be warning free to FromType) while still

        // ensuring we return a value clamped to the range of ToType.

        return v < static_cast<const BareFromType>(toLowest)

                ? toLowest

                : (v > static_cast<const BareFromType>(toHighest) ? toHighest

                                                                  : static_cast<ToType>(v));

    }

};
@@ -154,5 +157,10 @@ inline bool operator<(const Size& lhs, const Size& rhs) { 
    return lhs.height < rhs.height;

}



// Defining PrintTo helps with Google Tests.

static inline void PrintTo(const Size& size, ::std::ostream* os) {

    *os << "Size(" << size.width << ", " << size.height << ")";

}



} // namespace ui

} // namespace android

libs/ui/tests/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -111,6 +111,7 @@ cc_test { 


cc_test {

    name: "Size_test",

    test_suites: ["device-tests"],

    shared_libs: ["libui"],

    srcs: ["Size_test.cpp"],

    cflags: ["-Wall", "-Werror"],

libs/ui/tests/Size_test.cpp

+10 −0
Original line number Diff line number Diff line
@@ -19,8 +19,18 @@ 
#include <cmath>

#include <cstdlib>



#ifdef __clang__

#pragma clang diagnostic push

#pragma clang diagnostic error "-Wimplicit-int-float-conversion"

#pragma clang diagnostic error "-Wconversion"

#endif // __clang__



#include <ui/Size.h>



#ifdef __clang__

#pragma clang diagnostic pop

#endif // __clang__



#include <gtest/gtest.h>



namespace android {

libs/ui/tests/TEST_MAPPING

0 → 100644
+7 −0
Original line number Diff line number Diff line 
{

  "presubmit": [

    {

      "name": "Size_test"

    }

  ]

}