Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1824733002:
For *ToFixed, in debug mode, assert that the value is in range. (Closed)
DescriptionFor *ToFixed, in debug mode, assert that the value is in range.
Use SkFloatPinToFixed in SkTwoPointConicalGradient.cpp because it is failing in DM. fmalita is working on replacing this code with the 4f version, so I'm not bothering to fix it. (The beginnings of a real fix are in https://codereview.chromium.org/1767163003, which I do not plan to commit.)
BUG=skia:4632
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1824733002
Committed: https://skia.googlesource.com/skia/+/93dc33972cd6a418e84270298b856d2de08d9c1c
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Get rid of #undef. Don't #ifdef SK_DEBUG. #
Total comments: 1
Patch Set 3 : Whitespace. #Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #Patch Set 6 : Rebase. #
Dependent Patchsets: Messages
Total messages: 44 (21 generated)
Description was changed from ========== For *ToFixed, in debug mode, assert that the value is in range. Change to SkFloatPinToFixed in SkTwoPointConicalGradient.cpp because it is failing in DM. fmalita is working on replacing this code with the 4f version, so I'm not bothering to fix it. (The beginnings of a real fix are in https://codereview.chromium.org/1767163003, which I do not plan to commit.) BUG=skia:4632 ========== to ========== For *ToFixed, in debug mode, assert that the value is in range. Change to SkFloatPinToFixed in SkTwoPointConicalGradient.cpp because it is failing in DM. fmalita is working on replacing this code with the 4f version, so I'm not bothering to fix it. (The beginnings of a real fix are in https://codereview.chromium.org/1767163003, which I do not plan to commit.) BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824733002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824733002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
Patchset #1 (id:1) has been deleted
Description was changed from ========== For *ToFixed, in debug mode, assert that the value is in range. Change to SkFloatPinToFixed in SkTwoPointConicalGradient.cpp because it is failing in DM. fmalita is working on replacing this code with the 4f version, so I'm not bothering to fix it. (The beginnings of a real fix are in https://codereview.chromium.org/1767163003, which I do not plan to commit.) BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== For *ToFixed, in debug mode, assert that the value is in range. Use SkFloatPinToFixed in SkTwoPointConicalGradient.cpp because it is failing in DM. fmalita is working on replacing this code with the 4f version, so I'm not bothering to fix it. (The beginnings of a real fix are in https://codereview.chromium.org/1767163003, which I do not plan to commit.) BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824733002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824733002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
benjaminwagner@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h File include/core/SkFixed.h (left): https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... include/core/SkFixed.h:46: #ifdef SK_DEBUG Ditto. https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... include/core/SkFixed.h:36: static inline SkFixed SkFloatToFixed(float x) { It should be fine to use this definition in Debug and Release. SkASSERT does nothing in Release mode, and the rest will inline just like the macro. https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... include/core/SkFixed.h:165: #define SkFloatToFixed_Unsafe(x) SkFloatToFixed_arm(x) Can you double check that SkFloatToFixed still calls this SkFloatToFixed_arm() code? I don't remember enough about macros to know if it works or not, but I suspect it doesn't (the redefinition would only apply for later code, right?). https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... include/core/SkFixed.h:179: #ifdef SK_DEBUG ditto
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824733002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824733002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/22 at 12:38:03, mtklein wrote: > https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h > File include/core/SkFixed.h (left): > > https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... > include/core/SkFixed.h:46: #ifdef SK_DEBUG > Ditto. > > https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h > File include/core/SkFixed.h (right): > > https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... > include/core/SkFixed.h:36: static inline SkFixed SkFloatToFixed(float x) { > It should be fine to use this definition in Debug and Release. SkASSERT does nothing in Release mode, and the rest will inline just like the macro. > > https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... > include/core/SkFixed.h:165: #define SkFloatToFixed_Unsafe(x) SkFloatToFixed_arm(x) > Can you double check that SkFloatToFixed still calls this SkFloatToFixed_arm() code? I don't remember enough about macros to know if it works or not, but I suspect it doesn't (the redefinition would only apply for later code, right?). > > https://codereview.chromium.org/1824733002/diff/40001/include/core/SkFixed.h#... > include/core/SkFixed.h:179: #ifdef SK_DEBUG > ditto Hopefully all fixed. I couldn't think of a way to move the ARM-specific stuff to another file.
lgtm https://codereview.chromium.org/1824733002/diff/60001/include/core/SkFixed.h File include/core/SkFixed.h (right): https://codereview.chromium.org/1824733002/diff/60001/include/core/SkFixed.h#... include/core/SkFixed.h:32: #define SkFixedToFloat(x) ((x) * 1.52587890625e-5f) This may no longer need its new indentation.
benjaminwagner@google.com changed reviewers: + reed@google.com
Adding reed for API addition of SkFloatToFixed_Unsafe, SkDoubleToFixed_Unsafe, and SkFloatToFixed3232_Unsafe, although I'm hoping to move SkFixed.h to private.
Do we think there are any clients that use SkFixed? Can we eliminate them so we can make this file private? lgtm
On 2016/03/23 at 20:08:20, reed wrote: > Do we think there are any clients that use SkFixed? Can we eliminate them so we can make this file private? > > lgtm Still used in Android, but I plan to clean it up after https://codereview.chromium.org/1737693006.
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824733002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824733002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824733002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824733002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824733002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824733002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by benjaminwagner@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1824733002/#ps140001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824733002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824733002/140001
Message was sent while issue was closed.
Description was changed from ========== For *ToFixed, in debug mode, assert that the value is in range. Use SkFloatPinToFixed in SkTwoPointConicalGradient.cpp because it is failing in DM. fmalita is working on replacing this code with the 4f version, so I'm not bothering to fix it. (The beginnings of a real fix are in https://codereview.chromium.org/1767163003, which I do not plan to commit.) BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== For *ToFixed, in debug mode, assert that the value is in range. Use SkFloatPinToFixed in SkTwoPointConicalGradient.cpp because it is failing in DM. fmalita is working on replacing this code with the 4f version, so I'm not bothering to fix it. (The beginnings of a real fix are in https://codereview.chromium.org/1767163003, which I do not plan to commit.) BUG=skia:4632 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/93dc33972cd6a418e84270298b856d2de08d9c1c ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://skia.googlesource.com/skia/+/93dc33972cd6a418e84270298b856d2de08d9c1c
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/1868933004/ by fmalita@google.com. The reason for reverting is: Asserts in Blink rolls: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... STDERR: [1:1:0407/120919:1455366829:INFO:SkFixed.h(88)] ../../third_party/skia/include/private/SkFixed.h:88: fatal error: ""truncf(x * (1 << 16)) == static_cast<float>.
Message was sent while issue was closed.
Lets move that float->fixed function to be local to this .cpp |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
