0

Note: I am not using ARC

I have a UILabel with the following property: @property (nonatomic, retain) UILabel *someLabel; I am trying to set a custom setter. Would the following code cause a leak, since @property is actually calling retain as well?

- (void)setSomeLabel:(UILabel *)someLabel
{
    if (someLabel != self.someLabel) {
        [self.someLabel release];
        self.someLabel = [someLabel retain];
    }

    // some custom code here
}
darksky
  • 20,411
  • 61
  • 165
  • 254

4 Answers4

8

Note: I am not using ARC

You really, really should.


Your setter is an infinite loop. The call to self.someLabel = ... is the exact equivalent of [self setSomeLabel:...], which causes the loop.

A correct manual setter looks like this:

- (void)setSomeLabel:(UILabel *)someLabel
{
  [someLabel retain];
  [_someLabel release];
  _someLabel = someLabel;

  // some custom code here
}

There are other common patterns. A major question is whether "some custom code" should run if the object is reset to the same value. If not, then this pattern makes more sense:

- (void)setSomeLabel:(UILabel *)someLabel
{
  if (someLabel != _someLabel) {
    [_someLabel release];
    _someLabel = [someLabel retain];

    // some custom code here
  }
}
Rob Napier
  • 286,113
  • 34
  • 456
  • 610
  • in your first code sample, shouldn't `_someLabel = label;` be `_someLabel = someLabel;` instead? – AlexChaffee Apr 14 '13 at 00:26
  • Is this retain/release shuffle still needed under ARC? And is there any other magic that the synthesized setter might be doing (like with all the various modifiers like `strong` and `nonatomic` and whatnot) that we would have to mimic? – AlexChaffee Apr 14 '13 at 00:29
  • 1
    Fixed the typo. No, you don't need `retain` and `release` under ARC (in fact they are illegal). You can just assign normally to the ivar inside the setter. The property's modifier (strong/weak) will be applied to the ivar automatically if one of the accessors is not custom (if both are custom, you have to declare the ivar yourself). "nonatomic" just means "don't do the atomic stuff." I would never recommend exactly recreating "atomic;" it's better to use queue-based accessors (see http://stackoverflow.com/questions/15934036/avoid-this-dangling-pointer-with-arc) – Rob Napier Apr 14 '13 at 21:34
6

That code will lead your app to infinite loop as using self.someLabel causes calling method setSomeLabel:.

You could try the code below for your custom setter:

 @synthesize someLabel = _someLabel;
 - (void)setSomeLabel:(UILabel *)someLabel
 {
      if (someLabel != _someLabel)
      {
           [_someLabel release];
           _someLabel = [someLabel retain];
      }

      // custom code
 }

 - (void)dealloc
 { 
     [_someLabel release];
     // ... other releases
     [super dealloc];
 }
Nekto
  • 17,837
  • 1
  • 55
  • 65
0

No, This is perfectly fine as you are using a custom setter here.

@Property is just equivalent to declaring the accessor methods.

@Synthesize will generate the accessor methods based on the property declaration attributes ONLY IF the setter and/or getter are NOT IMPLEMENTED.

Shashikanth
  • 752
  • 5
  • 13
  • 1
    @InsertWittyName, what do you mean? While Shashikanth didn't catch the infinite loop in the OP's code, his statements about property and synthesize are correct. There's a very subtle difference between a property declaration and declaring the accessors directly, but it's very rarely important. – Rob Napier Sep 30 '12 at 21:44
  • @RobNapier Apologies, I was a little 'short' in my comment and shouldn't have been. I do think the explanations of property and synthesize could have been a bit more clear (they are described correctly however). – InsertWittyName Sep 30 '12 at 21:51
  • Voting up to counteract ridiculous down votes. In an overwhelming majority of contexts this answer is correct, putting aside missing the use of dot syntax within the setter. – Carl Veazey Sep 30 '12 at 22:19
0

I'm assuming you're not using ARC...

You are correct that you're over-retaining the someLabel that is passed in, also calling release on a property is not great!

I would use the instance variable instead of the property:

- (void)setSomeLabel:(UILabel *)someLabel
{
    if (someLabel != _someLabel) {
        [_someLabel release];
        _someLabel = [someLabel retain];
    }

    // some custom code here
}
InsertWittyName
  • 3,930
  • 1
  • 22
  • 27
  • There's no overretain there, just infinite recursion. (putting aside the pedantic point that infinite recursion will retain infinitely; but fixing the recursion fixes the retain). – Carl Veazey Sep 30 '12 at 22:12