Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2324083003:
Fix kLargestDrawQuadSize (Closed)
DescriptionFix kLargestDrawQuadSize
This should be the right fix.
The problem was that two of the classes were not listed in the
static asserts since they were already in the sizeof at the top, which
I missed when I changed it to YUVVideoDrawQuad.
BUG=644443
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/cbeebcb824092900816b2b35901fe42b6ff7f7fc
Cr-Commit-Position: refs/heads/master@{#417802}
Patch Set 1 #
Total comments: 5
Patch Set 2 : use a recursive variadic template #Patch Set 3 : use a recursive variadic template #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Fix kLargestDrawQuadSize This should be the right fix. The problem was that two of the classes were not listed in the static asserts since they were already in the sizeof at the top, which I missed when I changed it to YUVVideoDrawQuad. BUG=644443 ========== to ========== Fix kLargestDrawQuadSize This should be the right fix. The problem was that two of the classes were not listed in the static asserts since they were already in the sizeof at the top, which I missed when I changed it to YUVVideoDrawQuad. BUG=644443 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
hubbe@chromium.org changed reviewers: + danakj@chromium.org
Third time's a charm...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.cc File cc/quads/largest_draw_quad.cc (right): https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.... cc/quads/largest_draw_quad.cc:23: const size_t kLargestDrawQuadSize2 = sizeof(cc::RenderPassDrawQuad) > Can you make a helper constexpr method here to do the max (maybe the max of 3 things in one call?)
https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.cc File cc/quads/largest_draw_quad.cc (right): https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.... cc/quads/largest_draw_quad.cc:23: const size_t kLargestDrawQuadSize2 = sizeof(cc::RenderPassDrawQuad) > On 2016/09/09 23:15:23, danakj wrote: > Can you make a helper constexpr method here to do the max (maybe the max of 3 > things in one call?) I tried using std::max(), which I think is constexpr, but static_assert complained about that. I was contemplating using a define to make it smaller, but it didn't quite seem worth it.
https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.cc File cc/quads/largest_draw_quad.cc (right): https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.... cc/quads/largest_draw_quad.cc:23: const size_t kLargestDrawQuadSize2 = sizeof(cc::RenderPassDrawQuad) > On 2016/09/09 23:20:07, hubbe wrote: > On 2016/09/09 23:15:23, danakj wrote: > > Can you make a helper constexpr method here to do the max (maybe the max of 3 > > things in one call?) > > I tried using std::max(), which I think is constexpr, but static_assert > complained about that. I was contemplating using a define to make it smaller, > but it didn't quite seem worth it. std::max is constexpr in c++14: http://en.cppreference.com/w/cpp/algorithm/max
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.cc File cc/quads/largest_draw_quad.cc (right): https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.... cc/quads/largest_draw_quad.cc:23: const size_t kLargestDrawQuadSize2 = sizeof(cc::RenderPassDrawQuad) > On 2016/09/09 23:30:25, danakj wrote: > On 2016/09/09 23:20:07, hubbe wrote: > > On 2016/09/09 23:15:23, danakj wrote: > > > Can you make a helper constexpr method here to do the max (maybe the max of > 3 > > > things in one call?) > > > > I tried using std::max(), which I think is constexpr, but static_assert > > complained about that. I was contemplating using a define to make it smaller, > > but it didn't quite seem worth it. > > std::max is constexpr in c++14: http://en.cppreference.com/w/cpp/algorithm/max How about a recursive variadic template so we can get rid of the static_asserts completely?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.cc File cc/quads/largest_draw_quad.cc (right): https://codereview.chromium.org/2324083003/diff/1/cc/quads/largest_draw_quad.... cc/quads/largest_draw_quad.cc:23: const size_t kLargestDrawQuadSize2 = sizeof(cc::RenderPassDrawQuad) > On 2016/09/10 00:10:01, hubbe wrote: > On 2016/09/09 23:30:25, danakj wrote: > > On 2016/09/09 23:20:07, hubbe wrote: > > > On 2016/09/09 23:15:23, danakj wrote: > > > > Can you make a helper constexpr method here to do the max (maybe the max > of > > 3 > > > > things in one call?) > > > > > > I tried using std::max(), which I think is constexpr, but static_assert > > > complained about that. I was contemplating using a define to make it > smaller, > > > but it didn't quite seem worth it. > > > > std::max is constexpr in c++14: http://en.cppreference.com/w/cpp/algorithm/max > > How about a recursive variadic template so we can get rid of the static_asserts > completely? Is that going to be easier to read and maintain? I feel like a little helper method and static asserts is more obvious, but idk what that looks like.
Oh I see you did it
ok idk if that is like overly clever but it works and i think we can maintain that. LGTM
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 hubbe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix kLargestDrawQuadSize This should be the right fix. The problem was that two of the classes were not listed in the static asserts since they were already in the sizeof at the top, which I missed when I changed it to YUVVideoDrawQuad. BUG=644443 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Fix kLargestDrawQuadSize This should be the right fix. The problem was that two of the classes were not listed in the static asserts since they were already in the sizeof at the top, which I missed when I changed it to YUVVideoDrawQuad. BUG=644443 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/cbeebcb824092900816b2b35901fe42b6ff7f7fc Cr-Commit-Position: refs/heads/master@{#417802} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cbeebcb824092900816b2b35901fe42b6ff7f7fc Cr-Commit-Position: refs/heads/master@{#417802} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
