Dan Hipschman wrote:
var_t *field, } else if (is_ptr(type)) {
print_file(file, 2, "0x8,\t/* FC_LONG */\n");
unsigned char fc = (cont->type == RPC_FC_BOGUS_STRUCT? RPC_FC_POINTER: RPC_FC_LONG);print_file(file, 2, "0x%x,\t/* %s */\n", fc, string_of_type(fc)); *tfsoff += 1;
I sent a patch yesterday which does a similar thing to this chunk. I think we need to agree on which approach we're going to take so we can let Alexandre know which patch to commit.
On Tue, Sep 18, 2007 at 09:23:37AM +0100, Robert Shearman wrote:
Dan Hipschman wrote:
var_t *field, } else if (is_ptr(type)) {
print_file(file, 2, "0x8,\t/* FC_LONG */\n");
unsigned char fc = (cont->type == RPC_FC_BOGUS_STRUCT? RPC_FC_POINTER: RPC_FC_LONG);print_file(file, 2, "0x%x,\t/* %s */\n", fc, string_of_type(fc)); *tfsoff += 1;I sent a patch yesterday which does a similar thing to this chunk. I think we need to agree on which approach we're going to take so we can let Alexandre know which patch to commit.
Sorry, I missed your last three patches somehow. My patch not only uses RPC_FC_POINTER instead of RPC_FC_LONG here, but it also implements the pointer description section for complex structures. I actually left these as RPC_FC_LONG on purpose, until the pointer description part was finished. Isn't using RPC_FC_POINTER with a pointer description offset of zero just going to crash? It's not really any more correct in that case. There are no tests, so I'm not sure what it would do.
Dan Hipschman wrote:
On Tue, Sep 18, 2007 at 09:23:37AM +0100, Robert Shearman wrote:
Dan Hipschman wrote:
var_t *field, } else if (is_ptr(type)) {
print_file(file, 2, "0x8,\t/* FC_LONG */\n");
unsigned char fc = (cont->type == RPC_FC_BOGUS_STRUCT? RPC_FC_POINTER: RPC_FC_LONG);print_file(file, 2, "0x%x,\t/* %s */\n", fc, string_of_type(fc)); *tfsoff += 1;I sent a patch yesterday which does a similar thing to this chunk. I think we need to agree on which approach we're going to take so we can let Alexandre know which patch to commit.
Sorry, I missed your last three patches somehow. My patch not only uses RPC_FC_POINTER instead of RPC_FC_LONG here, but it also implements the pointer description section for complex structures.
Right, but I was really talking about the approach of passing a flag into write_member_types or passing the containing type in. I think it is slightly cleaner to do the former, but your patch works very well for the case I was trying to fix.
I actually left these as RPC_FC_LONG on purpose, until the pointer description part was finished. Isn't using RPC_FC_POINTER with a pointer description offset of zero just going to crash? It's not really any more correct in that case. There are no tests, so I'm not sure what it would do.
Sure, but it was slightly better than the existing code, which would have appeared to correctly unmarshal data, but fail in strange ways. If you want to always output correct format strings then you have to detect unimplemented cases and output an error!
On Tue, Sep 18, 2007 at 11:15:26PM +0100, Robert Shearman wrote:
Sorry, I missed your last three patches somehow. My patch not only uses RPC_FC_POINTER instead of RPC_FC_LONG here, but it also implements the pointer description section for complex structures.
Right, but I was really talking about the approach of passing a flag into write_member_types or passing the containing type in. I think it is slightly cleaner to do the former, but your patch works very well for the case I was trying to fix.
Well, it's such a small section of code it probably doesn't matter much either way. I tend to avoid boolean arguments because when you look at the call to the function and just see a bunch of TRUEs and FALSEs, it's hard to remember what they mean.
I actually left these as RPC_FC_LONG on purpose, until the pointer description part was finished. Isn't using RPC_FC_POINTER with a pointer description offset of zero just going to crash? It's not really any more correct in that case. There are no tests, so I'm not sure what it would do.
Sure, but it was slightly better than the existing code, which would have appeared to correctly unmarshal data, but fail in strange ways. If you want to always output correct format strings then you have to detect unimplemented cases and output an error!
I agree. It's much better to crash than to do the wrong thing without warning. In both cases, though, it was impossible to write tests since the output was wrong, so there wasn't much point worrying about it. If I had gone to the effort of doing anything about it, I would have just implemented what was needed. I just didn't get around to it until now.
Thanks for the comments.
Dan Hipschman wrote:
On Tue, Sep 18, 2007 at 11:15:26PM +0100, Robert Shearman wrote:
Sorry, I missed your last three patches somehow. My patch not only uses RPC_FC_POINTER instead of RPC_FC_LONG here, but it also implements the pointer description section for complex structures.
Right, but I was really talking about the approach of passing a flag into write_member_types or passing the containing type in. I think it is slightly cleaner to do the former, but your patch works very well for the case I was trying to fix.
Well, it's such a small section of code it probably doesn't matter much either way. I tend to avoid boolean arguments because when you look at the call to the function and just see a bunch of TRUEs and FALSEs, it's hard to remember what they mean.
Sure, it doesn't really matter and your set of patches implements more functionality. I'm happy to drop my patches in favour of yours.
-
Dan Hipschman -
Robert Shearman