LGTM Do you think this technique of pushing various function declaration into procstate.h is the ...
14 years, 11 months ago
(2009-12-10 19:39:35 UTC)
#2
LGTM
Do you think this technique of pushing various function declaration into
procstate.h is the right pattern? It is working, but I wonder if my notion that
the function names could be private, and only live in the table is misguided.
On 2009/12/10 02:24:26, Stephen White wrote:
>
On 2009/12/10 19:39:35, reed wrote: > LGTM > > Do you think this technique of ...
14 years, 11 months ago
(2009-12-10 19:58:15 UTC)
#3
On 2009/12/10 19:39:35, reed wrote:
> LGTM
>
> Do you think this technique of pushing various function declaration into
> procstate.h is the right pattern? It is working, but I wonder if my notion
that
> the function names could be private, and only live in the table is misguided.
The main thing I wanted to avoid was duplicating the tests in
SkBitmapProcState::chooseProcs().
It could be done using the table approach, we'd just have to pass an index
value, as the BlitRow PlatformProcs() functions do. The only downside is that
the index is kind of anonymous, and relies on setting up a table that matches
the one in the src dir. At least when there are only a few functions, checking
them by name (ptr value) is clearer in the code, IMHO.
OK. I don't have a better solution in mind, so we'll just keep up your ...
14 years, 11 months ago
(2009-12-10 21:14:39 UTC)
#4
OK. I don't have a better solution in mind, so we'll just keep up your pattern.
I just submitted the CL to enable the new builder by default (left the
old code-path in there for now. If it sticks, we can clean it up
later). I also uploaded new gm images to match. rev. 455
On Thu, Dec 10, 2009 at 2:58 PM, <senorblanco@chromium.org> wrote:
> On 2009/12/10 19:39:35, reed wrote:
>>
>> LGTM
>
>> Do you think this technique of pushing various function declaration
>
> into
>>
>> procstate.h is the right pattern? It is working, but I wonder if my
>
> notion that
>>
>> the function names could be private, and only live in the table is
>
> misguided.
>
> The main thing I wanted to avoid was duplicating the tests in
> SkBitmapProcState::chooseProcs().
>
> It could be done using the table approach, we'd just have to pass an
> index value, as the BlitRow PlatformProcs() functions do. The only
> downside is that the index is kind of anonymous, and relies on setting
> up a table that matches the one in the src dir. At least when there are
> only a few functions, checking them by name (ptr value) is clearer in
> the code, IMHO.
>
> http://codereview.appspot.com/171055
>
Issue 171055: More SSE2ification
(Closed)
Created 14 years, 11 months ago by Stephen White
Modified 13 years, 11 months ago
Reviewers: reed
Base URL: http://skia.googlecode.com/svn/trunk/
Comments: 0