On 2014/08/13 at 09:54:27, titzer wrote:
On 2014/08/13 09:43:47, Benedikt Meurer wrote:
> On 2014/08/13 at 09:41:02, titzer wrote:
> > On 2014/08/13 09:26:41, Benedikt Meurer wrote:
> > > On 2014/08/13 at 09:19:45, titzer wrote:
> > > > On 2014/08/13 09:14:13, Benedikt Meurer wrote:
> > > > >
> > >
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowering.cc
> > > > > File src/compiler/change-lowering.cc (right):
> > > > >
> > > > >
> > >
>
https://codereview.chromium.org/460593003/diff/20001/src/compiler/change-lowering.cc#newcode138
> > > > > src/compiler/change-lowering.cc:138: Reduction
> > > > > ChangeLowering<4>::ChangeBoolToBit(Node* val) {
> > > > > That's another option, sure. But I decided to do it this way.
> > > >
> > > > This is not necessary complexity. You undid a simple branch and
replaced
> it
> > > with template specialization on the pointer size. This an abuse of
templates
> > > with clear disadvantages and little or no benefit.
> > > >
> > > > Can we please go back to the simpler code?
> > >
> > > It was "a simple branch" because it was using the global state
implicitly
> (i.e.
> > > WordEqual and friends). Of course it's simple if you solve only a
subset
of
> the
> > > actual problem. Doing it right will require the "simple branch" in
many
> places,
> > > which is probably less readable than just having specialized
templates.
> > >
> > > Note that this class is not finished yet, and because we can now
actually
> test
> > > it, it'll be easy to refactor and remove the duplication.
> >
> > As I mentioned, a simple field on the class would suffice to get rid
of
the
> dependency on the kPointerSize. A template is still IMHO overkill.
>
> As I mentioned, you'll still need the simple branch on the simple
field. So
> that's just a matter of "template specialization" vs. "if
specialization".
>
> Let's see how things work out and if the template version is really much
more
> complex compare to the if version, then we can easily refactor that.
Can we please do the simpler thing first? We shouldn't be using templates
like
this until we are forced to really really good reasons.
Do the simpler thing first. It was already simple. Just go back to that.
BTW the MachineOperator builder already has a built-in word size, so you
don't
even need a field on this class, just check the word size of the one passed
in,
and in the test you can pass in one or the other.
As this is just a matter of personal preference, I'll do the actual work
first
and check if it makes sense based on the result.
https://codereview.chromium.org/460593003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
