Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2000853003:
SkXPS: clean up SkConstexprMath (Closed)
|
Created:
4 years, 7 months ago by hal.canary Modified:
4 years, 6 months ago Reviewers:
bungeman-skia CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkXPS: clean up SkConstexprMath
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000853003
Committed: https://skia.googlesource.com/skia/+/bfa9275968d11d459b30a485cedcb55c7fecf9d7
Patch Set 1 : 2016-05-22 (Sunday) 13:44:31 EDT #
Total comments: 10
Patch Set 2 : 2016-05-23 (Monday) 11:33:25 EDT #Patch Set 3 : 2016-05-23 (Monday) 11:47:20 EDT #Patch Set 4 : 2016-05-23 (Monday) 12:51:27 EDT #
Total comments: 4
Patch Set 5 : 2016-05-28 (Saturday) 12:57:46 EDT #
Total comments: 2
Patch Set 6 : 2016-05-30 (Monday) 10:22:51 EDT #
Total comments: 1
Messages
Total messages: 48 (28 generated)
Description was changed from ========== SkXPS: clean up SkConstexprMath . ========== to ========== SkXPS: clean up SkConstexprMath . GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000853003 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
The CQ bit was checked by halcanary@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/2000853003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000853003/200001
Patchset #1 (id:180001) has been deleted
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...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
The CQ bit was checked by halcanary@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/2000853003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000853003/260001
The CQ bit was unchecked by halcanary@google.com
halcanary@google.com changed reviewers: + bungeman@google.com
PTAL
Description was changed from ========== SkXPS: clean up SkConstexprMath . GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000853003 ========== to ========== SkXPS: clean up SkConstexprMath GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000853003 ==========
I think c++11 and SkTMax allows us to remove this file? https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMath.h File src/xps/SkConstexprMath.h (right): https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:14: /** Compile-time constant floor(log_{b}(n)) */ We don't want floor, we want ceiling. I think that's what the adding 1 is for. I think this is broken for '1', but was required for the simple form. https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:19: /** Compile-time constant number of base 10 digits in type t nit: 'type T' (capitalized, since that's how its actually written.) https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:22: constexpr size_t SkDigitsIn() { Actually, now that I look, "std::numeric_limits::digits10 + 1" seems to be the value we're after, and comes in constexpr. Can we use that instead? https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:27: constexpr size_t SkSMax(size_t a, size_t b) { If this is just used for size_t, just use SkTMax directly. https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkXPSDevice.cpp File src/xps/SkXPSDevice.cpp (right): https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:210: swprintf_s(buffer, size, L"/Documents/1/Metadata/%u.png", pageNum); Eck, now that I look at these, I think this needs to be at least _swprintf_s_l which takes a locale so we can make sure the numbers are formatted the way we expect. For example the locale fa_IR will produce non-arabic-indic digits here if it's the current locale on this thread. Anyhow, that's a separate issue from this.
https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMath.h File src/xps/SkConstexprMath.h (right): https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:14: /** Compile-time constant floor(log_{b}(n)) */ On 2016/05/23 15:04:49, bungeman-skia wrote: > We don't want floor, we want ceiling. I think that's what the adding 1 is for. I > think this is broken for '1', but was required for the simple form. function deleted. https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:19: /** Compile-time constant number of base 10 digits in type t On 2016/05/23 15:04:49, bungeman-skia wrote: > nit: 'type T' > > (capitalized, since that's how its actually written.) function deleted https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:22: constexpr size_t SkDigitsIn() { On 2016/05/23 15:04:49, bungeman-skia wrote: > Actually, now that I look, "std::numeric_limits::digits10 + 1" seems to be the > value we're after, and comes in constexpr. Can we use that instead? Done. https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkConstexprMat... src/xps/SkConstexprMath.h:27: constexpr size_t SkSMax(size_t a, size_t b) { On 2016/05/23 15:04:49, bungeman-skia wrote: > If this is just used for size_t, just use SkTMax directly. Done. https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkXPSDevice.cpp File src/xps/SkXPSDevice.cpp (right): https://codereview.chromium.org/2000853003/diff/260001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:210: swprintf_s(buffer, size, L"/Documents/1/Metadata/%u.png", pageNum); On 2016/05/23 15:04:49, bungeman-skia wrote: > Eck, now that I look at these, I think this needs to be at least _swprintf_s_l > which takes a locale so we can make sure the numbers are formatted the way we > expect. For example the locale fa_IR will produce non-arabic-indic digits here > if it's the current locale on this thread. Anyhow, that's a separate issue from > this. Acknowledged.
On 2016/05/23 15:04:49, bungeman-skia wrote: > I think c++11 and SkTMax allows us to remove this file? Done.
The CQ bit was checked by halcanary@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/2000853003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000853003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2000853003/diff/320001/src/xps/SkXPSDevice.cpp File src/xps/SkXPSDevice.cpp (right): https://codereview.chromium.org/2000853003/diff/320001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:205: constexpr size_t size = SkTMax<size_t>( Both parameters are size_t now, so it shouldn't need to be specified here. https://codereview.chromium.org/2000853003/diff/320001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:207: + static_cast<size_t>(std::numeric_limits<unsigned>::digits10 + 1), It's rather annoying that this indentation kinda hides where the two parameters are. Maybe it makes sense to create a file local template <typename T> static constexpr size_t sk_digits_in(T) { return static_cast<size_t>(std::numeric_limits<T>::digits10 + 1); } so that each parameter fits on one line (and also indicates intent). This also fixes the issue where in the new code 'unsigned' (a manual inlining of 'decltype(pageNum)') is being used instead of 'pageNum'.
The CQ bit was checked by halcanary@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/2000853003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000853003/340001
https://codereview.chromium.org/2000853003/diff/320001/src/xps/SkXPSDevice.cpp File src/xps/SkXPSDevice.cpp (right): https://codereview.chromium.org/2000853003/diff/320001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:205: constexpr size_t size = SkTMax<size_t>( On 2016/05/23 18:12:38, bungeman-skia wrote: > Both parameters are size_t now, so it shouldn't need to be specified here. Done. https://codereview.chromium.org/2000853003/diff/320001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:207: + static_cast<size_t>(std::numeric_limits<unsigned>::digits10 + 1), On 2016/05/23 18:12:38, bungeman-skia wrote: > It's rather annoying that this indentation kinda hides where the two parameters > are. Maybe it makes sense to create a file local > > template <typename T> static constexpr size_t sk_digits_in(T) { > return static_cast<size_t>(std::numeric_limits<T>::digits10 + 1); > } > > so that each parameter fits on one line (and also indicates intent). This also > fixes the issue where in the new code 'unsigned' (a manual inlining of > 'decltype(pageNum)') is being used instead of 'pageNum'. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2000853003/diff/340001/src/xps/SkXPSDevice.cpp File src/xps/SkXPSDevice.cpp (right): https://codereview.chromium.org/2000853003/diff/340001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:237: + static_cast<size_t>(std::numeric_limits<unsigned>::digits10 + 1); sk_digits_in(decltype(fCurrentPage))?
The CQ bit was checked by halcanary@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/2000853003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000853003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2000853003/diff/340001/src/xps/SkXPSDevice.cpp File src/xps/SkXPSDevice.cpp (right): https://codereview.chromium.org/2000853003/diff/340001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:237: + static_cast<size_t>(std::numeric_limits<unsigned>::digits10 + 1); On 2016/05/29 23:02:52, bungeman-skia wrote: > sk_digits_in(decltype(fCurrentPage))? Done.
lgtm https://codereview.chromium.org/2000853003/diff/360001/src/xps/SkXPSDevice.cpp File src/xps/SkXPSDevice.cpp (right): https://codereview.chromium.org/2000853003/diff/360001/src/xps/SkXPSDevice.cp... src/xps/SkXPSDevice.cpp:209: constexpr size_t size = SkTMax( I've gone back and forth now trying to figure out when to use 'static constexpr' and when to use just 'constexpr' for function locals. In this case it shouldn't make a (practical) difference, so we can leave it as is (it probably would make a difference if we needed to take the address of 'size').
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000853003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000853003/360001
Message was sent while issue was closed.
Description was changed from ========== SkXPS: clean up SkConstexprMath GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000853003 ========== to ========== SkXPS: clean up SkConstexprMath GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000853003 Committed: https://skia.googlesource.com/skia/+/bfa9275968d11d459b30a485cedcb55c7fecf9d7 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:360001) as https://skia.googlesource.com/skia/+/bfa9275968d11d459b30a485cedcb55c7fecf9d7 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
