Fix closure merge for external defined ones

I forgot to cover the case of external closures in the merge code for ADD. This patch fixes the problem.

[aconty, 2010-07-30 16:38:46]

Added support for merging external closures too

[aconty, 2010-07-30 17:29:46]

support for llvm side which I forgot in the previous patch

[ckulla, 2010-07-30 18:03:33]

http://codereview.appspot.com/1876042/diff/15001/16001
File src/include/oslclosure.h (right):

http://codereview.appspot.com/1876042/diff/15001/16001#newcode583
src/include/oslclosure.h:583: int   memsize;   ///< Memory size of the component
This isn't really needed. Its only used in the merge, where it could be looked
up from the closure registry.

http://codereview.appspot.com/1876042/diff/15001/16003
File src/liboslexec/builtin_closures.cpp (right):

http://codereview.appspot.com/1876042/diff/15001/16003#newcode189
src/liboslexec/builtin_closures.cpp:189: ss->register_closure (clinfo->name,
cid, clinfo->params, size, clinfo->prepare, generic_closure_setup, NULL,
Why not just have generic_closure_merge here? This would simplify the code in
add.

http://codereview.appspot.com/1876042/diff/15001/16004
File src/liboslexec/closure.cpp (right):

http://codereview.appspot.com/1876042/diff/15001/16004#newcode93
src/liboslexec/closure.cpp:93: if (A.is_builtin(ac)) {
You have duplicated code here, I think it would be simpler to just have a
function pointer take care of the generic case too.

http://codereview.appspot.com/1876042/diff/15001/16009
File src/liboslexec/oslexec_pvt.h (right):

http://codereview.appspot.com/1876042/diff/15001/16009#newcode494
src/liboslexec/oslexec_pvt.h:494: ASSERT((size_t)id < m_closure_table.size());
DASSERT

[aconty, 2010-07-31 00:14:17]

Addressed comments

[ckulla, 2010-07-31 01:28:52]

LGTM, just a few small notes.

http://codereview.appspot.com/1876042/diff/31001/32004
File src/liboslexec/closure.cpp (right):

http://codereview.appspot.com/1876042/diff/31001/32004#newcode95
src/liboslexec/closure.cpp:95: CompareClosureFunc compare = ss ?
closure->compare : NULL;
Is this null check really needed?

The only case it comes up is probably the closure_test program ... but since it
didn't crash on the line above, it probably isn't going through this code path
anyway.

http://codereview.appspot.com/1876042/diff/31001/32006
File src/liboslexec/llvm_instance.cpp (right):

http://codereview.appspot.com/1876042/diff/31001/32006#newcode1667
src/liboslexec/llvm_instance.cpp:1667: llvm::errs() << "val is " << *val << "
but Result is a " << Result.typespec().c_str() << ' ' << Result.mangled() <<
"\n";

Is this for debugging another issue?

[aconty, 2010-08-02 16:30:14]

Got it, I'll fix all these problems.

[aconty, 2010-08-02 16:31:29]

Addressed comments and debug clean up

[ckulla, 2010-08-02 17:36:51]

On 2010/08/02 16:31:29, aconty wrote:
> Addressed comments and debug clean up

LGTM

[aconty, 2010-08-02 17:45:41]

checked in to r791