Issue 1115: TypeCode_ptr base_typecode(TypeCode_ptr tc) (cxx_revision) Source: (, ) Nature: Revision Severity: Significant Summary: Summary: Look at this code and tell me what is wrong with it (CORBA:: left off for simplicity): So, now the question is: Inside the TypeCode_var::operator=(TypeCode_ptr tc) assignment operator, when tc == this, what should happen? Should this call release(this) to satisfy the requirements of code frag #3? Or should it be a noop to satisfy the requirements of code frag #4? Resolution: Close no change. The submitter of the issue misunderstood _var reference counting semantics. Revised Text: Actions taken: April 6, 1998: received issue June 13, 2000: closed issue Discussion: End of Annotations:===== Return-Path: Sender: jon@floorboard.com Date: Mon, 06 Apr 1998 19:35:27 -0700 From: Jonathan Biggar To: cxx_revision@omg.org, issues@omg.org Subject: Interesting piece of code?!?!?! Look at this code and tell me what is wrong with it (CORBA:: left off for simplicity): TypeCode_ptr base_typecode(TypeCode_ptr tc) // code frag #1 { TypeCode_var new_tc = TypeCode::_duplicate(tc); while (new_tc.kind() == tk_alias) new_tc = new_tc->content_type(); return new_tc._retn(); } Nothing? That's what I thought too. But call it like this: ORB_var orb = ...; // code frag #2 TypeCode_var tc = orb->create_string_tc(1); tc = base_typecode(tc.in()); This is the same as: tc = TypeCode::_duplicate(tc.in()); // code frag #3 Which looks identical to: tc = tc.in(); // code frag #4 from the point of view of the TypeCode_var assignment operator. So, now the question is: Inside the TypeCode_var::operator=(TypeCode_ptr tc) assignment operator, when tc == this, what should happen? Should this call release(this) to satisfy the requirements of code frag #3? Or should it be a noop to satisfy the requirements of code frag #4? Isn't this nasty? -- Jon Biggar Floorboard Software jon@floorboard.com jon@biggar.org Return-Path: X-Sender: vinoski@mail.boston.iona.ie Date: Tue, 07 Apr 1998 00:36:05 -0400 To: Jonathan Biggar From: Steve Vinoski Subject: Re: Interesting piece of code?!?!?! Cc: cxx_revision@omg.org, issues@omg.org At 07:35 PM 4/6/98 -0700, Jonathan Biggar wrote: >Look at this code and tell me what is wrong with it (CORBA:: left off >for simplicity): > >TypeCode_ptr base_typecode(TypeCode_ptr tc) // code frag #1 >{ > TypeCode_var new_tc = TypeCode::_duplicate(tc); > > while (new_tc.kind() == tk_alias) > new_tc = new_tc->content_type(); > > return new_tc._retn(); >} > >Nothing? That's what I thought too. But call it like this: > > ORB_var orb = ...; // code frag #2 > TypeCode_var tc = orb->create_string_tc(1); > > tc = base_typecode(tc.in()); > >This is the same as: > > tc = TypeCode::_duplicate(tc.in()); // code frag #3 > >Which looks identical to: > > tc = tc.in(); // code frag #4 > >from the point of view of the TypeCode_var assignment operator. > >So, now the question is: Inside the >TypeCode_var::operator=(TypeCode_ptr tc) >assignment operator, when tc == this, what should happen? > >Should this call release(this) to satisfy the requirements of code frag >#3? Or should it be a noop to satisfy the requirements of code frag #4? > >Isn't this nasty? Not really. If you look at your code and assume a reference-counting implementation, your create_string_tc() call returns a new TypeCode with a ref count of 1. You then pass it to the base_typecode() function which immediately duplicates it, yielding a ref count of 2. It's not an alias TypeCode, so the while loop does nothing. Because you return from the function by grabbing the _ptr from the _var via _retn(), the _var does not release its _ptr, so the ref count remains 2. You then assign that return value to a _var. The _var assignment operator is defined by the C++ mapping to release any _ptr it holds and adopt the new one. It releases, which drops the ref count to 1, and adopts, which holds the ref count at 1 until the _var goes out of scope. The case #4 that you show is clearly an error. As I said above, _var assignment requires releasing what you hold and adopting the _ptr passed in. You should follow this rule and write your impl to handle correct code like your examples #1, #2, and #3, not broken code like case #4. --steve Return-Path: X-Authentication-Warning: tigger.dstc.edu.au: michi owned process doing -bs Date: Tue, 7 Apr 1998 14:40:14 +1000 (EST) From: Michi Henning To: Jonathan Biggar cc: cxx_revision@omg.org, issues@omg.org Subject: Re: Interesting piece of code?!?!?! On Mon, 6 Apr 1998, Jonathan Biggar wrote: > Look at this code and tell me what is wrong with it (CORBA:: left off > for simplicity): > > TypeCode_ptr base_typecode(TypeCode_ptr tc) // code frag #1 > { > TypeCode_var new_tc = TypeCode::_duplicate(tc); > > while (new_tc.kind() == tk_alias) > new_tc = new_tc->content_type(); > > return new_tc._retn(); > } > > Nothing? That's what I thought too. But call it like this: > > ORB_var orb = ...; // code frag #2 > TypeCode_var tc = orb->create_string_tc(1); > > tc = base_typecode(tc.in()); > > This is the same as: > > tc = TypeCode::_duplicate(tc.in()); // code frag #3 > > Which looks identical to: > > tc = tc.in(); // code frag #4 > > from the point of view of the TypeCode_var assignment operator. > > So, now the question is: Inside the > TypeCode_var::operator=(TypeCode_ptr tc) > assignment operator, when tc == this, what should happen? > > Should this call release(this) to satisfy the requirements of code frag > #3? Or should it be a noop to satisfy the requirements of code frag #4? Hmmm... I'm not sure there is a problem here. Assignment to a _var from a pointer makes the _var take ownership. So, the following does the right thing: tc = TypeCode::_duplicate(tc.in()); If you write tc = tc.in(); that's simply a bug as far as I can see. This is because tc.in() retains ownership, but the assignment transfers ownership. In other words, if two type codes are different _vars, the following is simply wrong: TypeCode_var tc1 = ...; TypeCode_var tc2 = ...; tc1 = tc2.in(); // Trouble here, both tc1 and tc2 have ownership So, in general, any statement of the form x = y.in(); is wrong if both x and y are _var types. So, what about this? tc = tc; The fact that these things may be reference-counted does not matter, as far as I can see. For example, in a reference-counted implementation, tc = TypeCode::_duplicate(tc.in()); would probably do something like: TypeCode & operator=(const TypeCode &rhs) { increment_ref_count(rhs); decrement_ref_count(this); if (this != &rhs) // do assignment return *this; } Here, I think the usual rules about guarding against assignment to self are sufficient. Am I missing something? Cheers, Michi. -- Michi Henning +61 7 33654310 DSTC Pty Ltd +61 7 33654311 (fax) University of Qld 4072 michi@dstc.edu.au AUSTRALIA http://www.dstc.edu.au/BDU/staff/michi-henning.html Return-Path: X-Authentication-Warning: tigger.dstc.edu.au: michi owned process doing -bs Date: Tue, 7 Apr 1998 15:10:27 +1000 (EST) From: Michi Henning Reply-To: Michi Henning To: Jonathan Biggar cc: cxx_revision@omg.org, issues@omg.org Subject: Re: Interesting piece of code?!?!?! On Tue, 7 Apr 1998, Michi Henning wrote: > TypeCode & > operator=(const TypeCode &rhs) > { > increment_ref_count(rhs); > decrement_ref_count(this); > if (this != &rhs) > // do assignment > return *this; > } I could bite my bloody tongue off. The analysis was correct, but the code wasn't. No need to mess with the reference count of the rhs of course, because _duplicate has already done that. A standard test against self-assignment should be sufficient: TypeCode & operator=(const TypeCode &rhs) { decrement_ref_count(this); if (this != &rhs) { // Do assignment } return *this; } Sorry for the confusion, Michi. -- Michi Henning +61 7 33654310 DSTC Pty Ltd +61 7 33654311 (fax) University of Qld 4072 michi@dstc.edu.au AUSTRALIA http://www.dstc.edu.au/BDU/staff/michi-henning.html Return-Path: Sender: jon@floorboard.com Date: Mon, 06 Apr 1998 23:39:16 -0700 From: Jonathan Biggar To: Steve Vinoski CC: cxx_revision@omg.org, issues@omg.org Subject: Re: Interesting piece of code?!?!?! References: <3.0.5.32.19980407003605.008bb7e0@mail.boston.iona.ie> Steve Vinoski wrote: > Not really. If you look at your code and assume a reference-counting > implementation, your create_string_tc() call returns a new TypeCode > with a ref count of 1. You then pass it to the base_typecode() > function which immediately duplicates it, yielding a ref count of 2. > It's not an alias TypeCode, so the while loop does nothing. Because > you return from the function by grabbing the _ptr from the _var via > _retn(), the _var does not release its _ptr, so the ref count > remains > 2. You then assign that return value to a _var. The _var assignment > operator is defined by the C++ mapping to release any _ptr it holds > and adopt the new one. It releases, which drops the ref count to 1, > and adopts, which holds the ref count at 1 until the _var goes out > of > scope. > > The case #4 that you show is clearly an error. As I said above, _var > assignment requires releasing what you hold and adopting the _ptr > passed in. You should follow this rule and write your impl to handle > correct code like your examples #1, #2, and #3, not broken code like > case #4. Ok, I'll agree with you and Michi, but it does suggest that the spec should be beefed up some to make it clear what is the right behavior. -- Jon Biggar Floorboard Software jon@floorboard.com jon@biggar.org