Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453473004:
Avoid separate allocation of SkImageCacherator (Closed)
DescriptionAvoid separate allocation of SkImageCacherator
Embed directly in SkImage_Generator, and add a helper to handle param
validation.
R=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2453473004
Committed: https://skia.googlesource.com/skia/+/7929e3ae76719f65111e4fe7a2f2444d53228c2b
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : cleanup #Patch Set 4 : pass validator to ctor #
Total comments: 2
Messages
Total messages: 22 (12 generated)
Description was changed from ========== Avoid a heap-allocating SkImageCacherator Embed directly in SkImage_Generator, and add a helper to handle param validation. R=reed@google.com ========== to ========== Avoid a heap-allocating SkImageCacherator Embed directly in SkImage_Generator, and add a helper to handle param validation. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2453473004 ==========
Description was changed from ========== Avoid a heap-allocating SkImageCacherator Embed directly in SkImage_Generator, and add a helper to handle param validation. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2453473004 ========== to ========== Avoid separate allocation of SkImageCacherator Embed directly in SkImage_Generator, and add a helper to handle param validation. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2453473004 ==========
If we're going to add an allocation with the shared generator thingie, maybe its worth trying this to save one for the cacherator.
The CQ bit was checked by fmalita@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: This issue passed the CQ dry run.
Saving the allocation seems good, but the temp constructor+operator() seems
tricky. I presume the goal was to share (external) code that validated the
arguments before calling a constructor..?
why not just
bool validate_args(...) {
}
as a shared function?
On 2016/10/26 14:48:47, reed1 wrote:
> Saving the allocation seems good, but the temp constructor+operator() seems
> tricky. I presume the goal was to share (external) code that validated the
> arguments before calling a constructor..?
Yeah, pretty much.
>
> why not just
>
> bool validate_args(...) {
> }
>
> as a shared function?
That will work too, but we're not just validating - we're also processing the
input to produce the ctor args => it seemed desirable to keep these args packed
together, if only to save some repetition.
I'll refactor as you suggested, let's see how that looks.
On 2016/10/26 15:05:01, f(malita) wrote:
> On 2016/10/26 14:48:47, reed1 wrote:
> > Saving the allocation seems good, but the temp constructor+operator() seems
> > tricky. I presume the goal was to share (external) code that validated the
> > arguments before calling a constructor..?
>
> Yeah, pretty much.
>
> >
> > why not just
> >
> > bool validate_args(...) {
> > }
> >
> > as a shared function?
>
> That will work too, but we're not just validating - we're also processing the
> input to produce the ctor args => it seemed desirable to keep these args
packed
> together, if only to save some repetition.
>
> I'll refactor as you suggested, let's see how that looks.
They're sort of packaged, but then we unpack them to call the constructor. If
the constructor took a validator obj... (just a thought)
On 2016/10/26 15:14:46, reed1 wrote: > They're sort of packaged, but then we unpack them to call the constructor. If > the constructor took a validator obj... (just a thought) Yeah, I think that looks better. PTAL.
The CQ bit was checked by fmalita@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: This issue passed the CQ dry run.
lgtm w/ request for some assurance that the Validator is always valid https://codereview.chromium.org/2453473004/diff/60001/src/image/SkImage_Gener... File src/image/SkImage_Generator.cpp (right): https://codereview.chromium.org/2453473004/diff/60001/src/image/SkImage_Gener... src/image/SkImage_Generator.cpp:103: return validator ? sk_make_sp<SkImage_Generator>(&validator) : nullptr; Can we force somebody to assert (Validator?) if we try to "use" one that isn't valid? I'm trying to enforce the caller's responsibility to check the predicate before using.
https://codereview.chromium.org/2453473004/diff/60001/src/image/SkImage_Gener... File src/image/SkImage_Generator.cpp (right): https://codereview.chromium.org/2453473004/diff/60001/src/image/SkImage_Gener... src/image/SkImage_Generator.cpp:103: return validator ? sk_make_sp<SkImage_Generator>(&validator) : nullptr; On 2016/10/27 13:45:19, reed1 wrote: > Can we force somebody to assert (Validator?) if we try to "use" one that isn't > valid? I'm trying to enforce the caller's responsibility to check the predicate > before using. The SkImageCacherator ctor (the only "user" of validator?) does assert that the generator ptr is non-null, which is equivalent to checking the validator. The reason we can't just check the validator at that point is because we're releasing the unique_ptr in the initializer list, so by the time we're reaching the ctor body the validator is no longer "valid". Does that sound sufficient, or do you have something else in mind?
On 2016/10/27 14:17:07, f(malita) wrote: > https://codereview.chromium.org/2453473004/diff/60001/src/image/SkImage_Gener... > File src/image/SkImage_Generator.cpp (right): > > https://codereview.chromium.org/2453473004/diff/60001/src/image/SkImage_Gener... > src/image/SkImage_Generator.cpp:103: return validator ? > sk_make_sp<SkImage_Generator>(&validator) : nullptr; > On 2016/10/27 13:45:19, reed1 wrote: > > Can we force somebody to assert (Validator?) if we try to "use" one that isn't > > valid? I'm trying to enforce the caller's responsibility to check the > predicate > > before using. > > The SkImageCacherator ctor (the only "user" of validator?) does assert that the > generator ptr is non-null, which is equivalent to checking the validator. The > reason we can't just check the validator at that point is because we're > releasing the unique_ptr in the initializer list, so by the time we're reaching > the ctor body the validator is no longer "valid". > > Does that sound sufficient, or do you have something else in mind? sgtm
The CQ bit was checked by fmalita@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.
Description was changed from ========== Avoid separate allocation of SkImageCacherator Embed directly in SkImage_Generator, and add a helper to handle param validation. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2453473004 ========== to ========== Avoid separate allocation of SkImageCacherator Embed directly in SkImage_Generator, and add a helper to handle param validation. R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2453473004 Committed: https://skia.googlesource.com/skia/+/7929e3ae76719f65111e4fe7a2f2444d53228c2b ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/7929e3ae76719f65111e4fe7a2f2444d53228c2b |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
