Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274863002:
sk_at_scope_end (Closed)
Descriptionsk_at_scope_end
The scoped WebP cleanup in https://codereview.chromium.org/2269333002/ got me
thinking we're missing this handy tool.
We've got std::unique_ptr and its variants (SkAutoTCallVProc, etc.) and of
course all the SkAutoThisAndThat, but they're all tied to pointers or particular
use cases, and none are quite so pithy.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2274863002
Patch Set 1 #Patch Set 2 : SK_UNUSED #Patch Set 3 : use it! #
Total comments: 10
Messages
Total messages: 23 (16 generated)
Description was changed from ========== sk_at_scope_end This seems like a handy tool that we're missing. We've got std::unique_ptr and its variants (SkAutoTCallVProc, etc.) and of course all the SkAutoThisAndThat, but they're all tied to pointers, and none are quite so pithy. BUG=skia: ========== to ========== sk_at_scope_end This seems like a handy tool that we're missing. We've got std::unique_ptr and its variants (SkAutoTCallVProc, etc.) and of course all the SkAutoThisAndThat, but they're all tied to pointers, and none are quite so pithy. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2274863002 ==========
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== sk_at_scope_end This seems like a handy tool that we're missing. We've got std::unique_ptr and its variants (SkAutoTCallVProc, etc.) and of course all the SkAutoThisAndThat, but they're all tied to pointers, and none are quite so pithy. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2274863002 ========== to ========== sk_at_scope_end The scoped WebP cleanup in https://codereview.chromium.org/2269333002/ got me thinking we're missing this handy tool. We've got std::unique_ptr and its variants (SkAutoTCallVProc, etc.) and of course all the SkAutoThisAndThat, but they're all tied to pointers or particular use cases, and none are quite so pithy. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2274863002 ==========
mtklein@chromium.org changed reviewers: + bungeman@google.com, msarett@google.com
Any interest in trying this out on the webp code now that it has landed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/24 at 14:37:12, msarett wrote: > Any interest in trying this out on the webp code now that it has landed? But of course! Please see patch set 3.
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
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/2274863002/diff/40001/include/core/SkPostConf... File include/core/SkPostConfig.h (right): https://codereview.chromium.org/2274863002/diff/40001/include/core/SkPostConf... include/core/SkPostConfig.h:249: #define SK_UNUSED __pragma(warning(suppress:4189)) Won't this suppress the warning for the entire rest of the translation unit once used? https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.cpp File src/codec/SkWebpCodec.cpp (right): https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:113: if (demux && WebPDemuxGetChunk(demux, "ICCP", 1, &chunkIterator)) { Is it valid to call WebPDemuxReleaseChunkIterator on an uninitialized WebPChunkIterator? If demux is false (and I'm assuming it can be, since we're testing it here), then WebPDemuxGetChunk is never called to initialize chunkIterator, and even if it is, WebPDemuxGetChunk can fail and again chunkIterator isn't called. Is there a need for this scoped thing at all? shouldn't WebPDemuxReleaseChunkIterator be called on chunkIterator only inside this 'if' block right after calling SkColorSpace::NewICC?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/2274863002/diff/40001/include/core/SkPostConf... File include/core/SkPostConfig.h (right): https://codereview.chromium.org/2274863002/diff/40001/include/core/SkPostConf... include/core/SkPostConfig.h:249: #define SK_UNUSED __pragma(warning(suppress:4189)) On 2016/08/24 15:18:17, bungeman-skia wrote: > Won't this suppress the warning for the entire rest of the translation unit once > used? I don't think so. As far as I can tell, suppress ends at the end of the current line, and disable would turn it off for the rest of the translation unit. https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.cpp File src/codec/SkWebpCodec.cpp (right): https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:113: if (demux && WebPDemuxGetChunk(demux, "ICCP", 1, &chunkIterator)) { On 2016/08/24 15:18:17, bungeman-skia wrote: > Is it valid to call WebPDemuxReleaseChunkIterator on an uninitialized > WebPChunkIterator? If demux is false (and I'm assuming it can be, since we're > testing it here), then WebPDemuxGetChunk is never called to initialize > chunkIterator, and even if it is, WebPDemuxGetChunk can fail and again > chunkIterator isn't called. > > Is there a need for this scoped thing at all? shouldn't > WebPDemuxReleaseChunkIterator be called on chunkIterator only inside this 'if' > block right after calling SkColorSpace::NewICC? Gonna have to leave this one to Matt. I don't think this CL changes anything related to this question.
https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.cpp File src/codec/SkWebpCodec.cpp (right): https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:113: if (demux && WebPDemuxGetChunk(demux, "ICCP", 1, &chunkIterator)) { On 2016/08/24 16:52:58, mtklein wrote: > On 2016/08/24 15:18:17, bungeman-skia wrote: > > Is it valid to call WebPDemuxReleaseChunkIterator on an uninitialized > > WebPChunkIterator? If demux is false (and I'm assuming it can be, since we're > > testing it here), then WebPDemuxGetChunk is never called to initialize > > chunkIterator, and even if it is, WebPDemuxGetChunk can fail and again > > chunkIterator isn't called. > > > > Is there a need for this scoped thing at all? shouldn't > > WebPDemuxReleaseChunkIterator be called on chunkIterator only inside this 'if' > > block right after calling SkColorSpace::NewICC? > > Gonna have to leave this one to Matt. > I don't think this CL changes anything related to this question. I'm following the same pattern as chromium here. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... IMO, it makes sense for WebPDemuxReleaseChunkIterator to not fail even if we haven't managed to read any chunks. That's the behavior we get from jpeg_destory() etc. Amusingly, this function actually does nothing anyway, so it seems that it doesn't matter. https://cs.chromium.org/chromium/src/third_party/libwebp/demux/demux.c?sq=pac... I think this is fine as is. If we decide we want to fix, I'll handle it in a follow-up CL.
https://codereview.chromium.org/2274863002/diff/40001/include/private/SkTempl... File include/private/SkTemplates.h (right): https://codereview.chromium.org/2274863002/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:506: class SkScoped { As far as naming goes, the current proposal (p0052) seems to lean toward scope_exit / make_scope_exit. I think the original ScopeGuard class used AT_SCOPE_EXIT as the name of the define. https://codereview.chromium.org/2274863002/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:527: #define SCOPED_NAME(n) SCOPED_NAMEx(n) These names seem a bit generic for something that will end up in other peoples translation units. Seems they should be SK_ prefixed, or use SK_MACRO_APPEND_LINE (or the like) instead. https://codereview.chromium.org/2274863002/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:529: #define sk_at_scope_end(stmt) \ You evil person making this define look like a function call. https://codereview.chromium.org/2274863002/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:530: SK_UNUSED auto&& SCOPED_NAME(__COUNTER__) = sk_make_scoped([&] { stmt; }) Looks like we've been using SK_MACRO_APPEND_LINE for this sort of thing. __COUNTER__ is of course cooler than __LINE__, but I don't think there's any reason SK_MACRO_APPEND_LINE shouldn't use __COUNTER__. https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.cpp File src/codec/SkWebpCodec.cpp (right): https://codereview.chromium.org/2274863002/diff/40001/src/codec/SkWebpCodec.c... src/codec/SkWebpCodec.cpp:111: WebPChunkIterator chunkIterator; Now that we've established webp has done a silly thing here, this seems more like it wants unique_resource from p0052 instead of scope_exit. In other words, it would be nice if the type itself managed the lifetime, instead of some external guard. Something like unique_resource<WebPChunkIterator, [](WebPChunkIterator& r){WebPDemuxReleaseChunkIterator(&r)}> chunkIterator; Ok, so maybe that doesn't look quite as nice, but that's mostly because we want a value type instead of a handle type. A generic adapter for that sort of thing could look like template <typename T, typename D> using unique_value = unique_resource<T, [](T& t){D(&t)}>; unique_value<WebPChunkIterator, WebPDemuxReleaseChunkIterator> chunkIterator; Note that all these variations here should turn into the same machine code, but with something like 'unique_value' I can see at a glance from the type that this is a scoped value of a given type and a specified thing will run on it when it goes out of scope. Spreading this out over two statements seems like more mental overhead. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
