Removing a 'dead code' warning under LLVM compiler 2.0. #19

Closed
wants to merge 1 commit into from

6 participants

@jparise

LLVM flags these lines as unnecessary assignments because anObject is being
both retained and then assigned back to itself. Removing the assignment
addresses the warning without changing the code's existing behavior.

Jon Parise Removing a 'dead code' warning under LLVM compiler 2.0.
LLVM flags these lines as unnecessary assignments because anObject is being
both retained and then assigned back to itself.  Removing the assignment
addresses the warning without changing the code's existing behavior.
fc5aafa
@jparise

I've read your comments on issue #1 regarding your feelings on changing the code to hint the static analyzer. While I agree with that stance, I think addressing this particular issue as per the diff seems worthwhile as the intent of the existing code remains unchanged and no less clear.

@jj0b

I've changed this for myself locally and am also of the opinion that it should be changed in the public repo. I don't think the assignment is harming anything but why have it if it serves no purpose.

@jasongregori

agreed

@johnezang johnezang added a commit that referenced this pull request May 21, 2011
@johnezang Workarounds for issue #19 (the clang stuff) and issue #23. For issue #23
, the code in the collection classes `+load` was removed and placed in a function with the `__attribute__ ((constructor))` attribute.  This is to work around an apparent bug when building JSONKit as a static library for iOS targets.  @ohhorob also opened a bug with apple- # 9461567.
f403357
@johnezang
Owner

This should be "fixed" in johnezang/JSONKit@f403357.

@johnezang johnezang closed this May 21, 2011
@johnezang johnezang was assigned May 21, 2011
@jasongregori

just curious: is there any reason not to just replace the line entirely instead of using the ifdef?

@johnezang
Owner

"The experienced programmer looks both ways before crossing a one-way street."

In a nutshell, the assignment is completely harmless if -retain returns the same object that retain was sent to.

On the other hand, if someone has overridden -retain to, for whatever reason, return a different object, then it can make a big difference.

The vast majority of the time, -retain will return the same object. One could even make a case that the current documentation says that -retain should / must return the same object. But people are still free to override -retain and do something different. Better safe than sorry, I say.

@johnezang
Owner

Also, this particular case is an excellent example of why I originally wrote this in the README.md:

The author requests that you do not file a bug report with JSONKit regarding problems reported by the clang static analyzer unless you first manually verify that it is an actual, bona-fide problem with JSONKit and, if appropriate, is not "legal" C code as defined by the C99 language specification. If the clang static analyzer is reporting a problem with JSONKit that is not an actual, bona-fide problem and is perfectly legal code as defined by the C99 language specification, then the appropriate place to file a bug report or complaint is with the developers of the clang static analyzer.

Note: I wrote this before the #ifdef hack was available.

What is the benefit to issuing a warning in this particular case? Will the "useless assignment" result in buggy behavior? Realistically, is it even possible for this "useless assignment" to change the behavior of the program? Are things improved if you remove the useless assignment in any way?

IMHO, no. This clang warning does nothing other than waste a programmers time in investigating whether or not there is a real problem, and when you realize there's nothing wrong, it's yet another false positive warning that you need to remember to ignore.

@jasongregori
@johnezang
Owner

n/p. While I have certain feelings towards these kinds of warnings from clang, I understand that others prefer to have "analyzer warning-free builds", or work in environments in which there is an edict that this must be so. Thankfully the latest versions of clang have a way (via #ifdef) to work around some of these spurious warnings.

@tonyarnold

I'm not sure I understand under what circumstances anyone would ever override retain such that your #ifdef would be necessary. In my experience, it's completely valid to just say [anObject retain]; without an assignment.

@jasongregori jasongregori added a commit to jasongregori/JSONKit that referenced this pull request Sep 23, 2011
@johnezang Workarounds for issue #19 (the clang stuff) and issue #23. For issue #23
, the code in the collection classes `+load` was removed and placed in a function with the `__attribute__ ((constructor))` attribute.  This is to work around an apparent bug when building JSONKit as a static library for iOS targets.  @ohhorob also opened a bug with apple- # 9461567.
09e1139
@johnezang
Owner

@tonyarnold, the problem is that while it may be difficult for you or I to imagine "under what circumstances anyone would ever override retain", that doesn't mean that it can't be done. A failure of imagination != impossible, or as the quantum physicists are known to say "That which is not strictly forbidden, can and will happen."

As a practical example, imagine a NSString like class that overrides retain such that the returned pointer is not may not be the same pointer to which the retain message was sent.

Why would it do this? When a NSString is retained, the implementation checks to see if there is a canonical object for that string. The set of canonical objects is created dynamically on an as needed basis. This is known as string interning, and one of the reasons that it is done is that it can save quite a bit of memory since instead of 50 - 200 string objects that all contain exactly the same content (i.e., @"0"), there are just a few strings objects, and the stranglers would tend to decrease over time. This example glosses over some obvious implementation details, but it's meant to illustrate that there might be genuine reasons why you might want to return a different pointer.

Now, it makes zero practical difference if you write-

obj = [obj retain];

... compared to ...

[obj retain];

... except in the case when some thing does actually return a pointer such that obj != [obj retain]. Given this, it's obvious what you should do: code defensively. If the pointer returned by retain is identical to the pointer that the message was sent to, obj = [obj retain] has exactly the same semantics as [obj retain].

In other words, obj = [obj retain] is a strict superset of the behavior and semantics of [obj retain], which is to say–

    [obj retain]obj = [obj retain].

@tonyarnold

Fair enough — you're very right about defensive coding, but still — it's a doozy of an edge case. Good explanation! Thanks for taking the time to write it :)

@Coeur

"As a convenience, retain returns self because it may be used in nested expressions."

retain returns self to help writing less code. Assigning self to self and writing more code is a bad practice. jparise's simple solution is better than johnezang's elaborate solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment