General cleanup to remove friends and comply to this-> policy (issue1847045)
e...@4geeks.de
Message:
I need someone to review this patch, or give me a "commit for
free"-card.
Description:
This patches changes most opcode files to comply to the this-> policy we
have to access member variables in C++.
It also removes all friend-statements from llvm_fbuilder.h
Please review this at http://codereview.appspot.com/1847045/show
Affected files:
M JIT/llvm_compile.cc
M JIT/llvm_fbuilder.cc
M JIT/llvm_fbuilder.h
M JIT/opcodes/attributes.cc
M JIT/opcodes/attributes.h
M JIT/opcodes/binops.cc
M JIT/opcodes/block.cc
M JIT/opcodes/block.h
M JIT/opcodes/call.cc
M JIT/opcodes/call.h
M JIT/opcodes/closure.cc
M JIT/opcodes/closure.h
M JIT/opcodes/cmpops.cc
M JIT/opcodes/cmpops.h
M JIT/opcodes/container.cc
M JIT/opcodes/container.h
M JIT/opcodes/control.cc
M JIT/opcodes/control.h
M JIT/opcodes/globals.cc
M JIT/opcodes/globals.h
M JIT/opcodes/locals.cc
M JIT/opcodes/locals.h
M JIT/opcodes/loop.cc
M JIT/opcodes/loop.h
M JIT/opcodes/name.cc
M JIT/opcodes/slice.cc
M JIT/opcodes/stack.cc
M JIT/opcodes/unaryops.cc
reid.k...@gmail.com
http://codereview.appspot.com/1847045/diff/1/24
File JIT/opcodes/attributes.cc (right):
http://codereview.appspot.com/1847045/diff/1/24#newcode92
JIT/opcodes/attributes.cc:92: llvm_data_(fbuilder->llvm_data())
It seems like for all of these opcode helper classes it would be good to
have a common base class to hold these fields and the BuilderT typedef.
The fields would be made protected, and all these initialization lists
would just call the base constructor.
This would also help if/when we need to add fields to roughly all of the
opcodes.
http://codereview.appspot.com/1847045/diff/1/24#newcode205
JIT/opcodes/attributes.cc:205: BasicBlock *do_store =
this->state_->CreateBasicBlock("STORE_ATTR_do_store");
80 cols
http://codereview.appspot.com/1847045/diff/1/24#newcode345
JIT/opcodes/attributes.cc:345:
ConstantInt::get(PyTypeBuilder<long>::get(this->fbuilder_->context()),
We use the context all over for getting/creating types. I think it
would be best to move it into the opcode base class I described above so
we can replace 'this->fbuilder_->context()' with 'this->context_'. The
PyTypeBuilder lines are the ones that tend to force us to line wrap.
http://codereview.appspot.com/1847045/diff/1/11
File JIT/opcodes/closure.cc (right):
http://codereview.appspot.com/1847045/diff/1/11#newcode66
JIT/opcodes/closure.cc:66:
PyTypeBuilder<Py_ssize_t>::get(this->fbuilder_->context()),
num_defaults);
80 cols
http://codereview.appspot.com/1847045/diff/1/11#newcode68
JIT/opcodes/closure.cc:68: this->state_->GetGlobalFunction<PyObject
*(Py_ssize_t)>("PyTuple_New");
80 cols
http://codereview.appspot.com/1847045/diff/1/11#newcode135
JIT/opcodes/closure.cc:135:
ConstantInt::get(PyTypeBuilder<int>::get(this->fbuilder_->context()),
index));
80 cols
e...@4geeks.de
http://codereview.appspot.com/1847045/diff/1/24#newcode92
JIT/opcodes/attributes.cc:92: llvm_data_(fbuilder->llvm_data())
On 2010/07/27 15:21:53, Reid Kleckner wrote:
> It seems like for all of these opcode helper classes it would be good
to have a
> common base class to hold these fields and the BuilderT typedef. The
fields
> would be made protected, and all these initialization lists would just
call the
> base constructor.
> This would also help if/when we need to add fields to roughly all of
the
> opcodes.
Maybe I'll do that in a later patch. Problem is, that our coding
guidelines do not allow protected members, so I would have to rename
everything again, to use accessors. Ugh.
http://codereview.appspot.com/1847045/diff/1/24#newcode345
JIT/opcodes/attributes.cc:345:
ConstantInt::get(PyTypeBuilder<long>::get(this->fbuilder_->context()),
On 2010/07/27 15:21:53, Reid Kleckner wrote:
> We use the context all over for getting/creating types. I think it
would be
> best to move it into the opcode base class I described above so we can
replace
> 'this->fbuilder_->context()' with 'this->context_'. The PyTypeBuilder
lines are
> the ones that tend to force us to line wrap.
IMHO we don't use TypeBuilder that often ... If it is used more then
twice you can always create a local reference.
Design decision!
http://codereview.appspot.com/1847045/diff/1/11
File JIT/opcodes/closure.cc (right):
http://codereview.appspot.com/1847045/diff/1/11#newcode66
JIT/opcodes/closure.cc:66:
PyTypeBuilder<Py_ssize_t>::get(this->fbuilder_->context()),
num_defaults);
On 2010/07/27 15:21:53, Reid Kleckner wrote:
> 80 cols
Done.
http://codereview.appspot.com/1847045/diff/1/11#newcode68
JIT/opcodes/closure.cc:68: this->state_->GetGlobalFunction<PyObject
*(Py_ssize_t)>("PyTuple_New");
On 2010/07/27 15:21:53, Reid Kleckner wrote:
> 80 cols
Done.
http://codereview.appspot.com/1847045/diff/1/11#newcode135
JIT/opcodes/closure.cc:135:
ConstantInt::get(PyTypeBuilder<int>::get(this->fbuilder_->context()),
index));
On 2010/07/27 15:21:53, Reid Kleckner wrote:
> 80 cols
Done.
Reid Kleckner
Reid