Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve minor static analysis issues #770

Merged
merged 3 commits into from
Jun 29, 2016

Conversation

tonyarnold
Copy link
Contributor

Not a major issue by any definition of the word, but we're seeing static analysis issues come through in our build logs when including RxSwift as a subproject.

I could use some guidance here on your project's style and approach: there are 11 additional static analysis warnings as follows:

RxSwift/RxCocoa/Common/_RXObjCRuntime.m:599:16: warning: Potential null dereference.  According to coding standards in 'Creating and Returning NSError Objects' the parameter may be null
        *error = [NSError errorWithDomain:RXObjCRuntimeErrorDomain
        ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I see that you've annotated the NSError *__autoreleasing *__nonnull, however if you read Apple's blog post on nullability, they specifically say:

The particular type NSError ** is so often used to return errors via method parameters that it is always assumed to be a nullable pointer to a nullable NSError reference.

So these issues are only going to go away if the code is changed to treat them as nullable. I'm happy to make this change (something as simple as an early return, or a parameter assertion), but I wasn't sure which approach you'd prefer.

It's also worth noting that people have filed radars against this issue, however I'd like to see this resolved sooner rather than later.

@kzaher
Copy link
Member

kzaher commented Jun 28, 2016

Hi @tonyarnold ,

.... just noticed this one.

I think maybe the smart thing would be to add something like

#define RX_THROW(errorValue)  if (error) { *error = (errorValue); }

and use it.

RX_THROW([NSError errorWithDomain:RXObjCRuntimeErrorDomain

Let me know what you think.

@tonyarnold
Copy link
Contributor Author

OK, I've added a simple macro and documentation in e4219c8.

I considered using a static inline function, but the macro was simplest.

@RxPullRequestBot
Copy link

RxPullRequestBot commented Jun 29, 2016

        1 Warning
    
  </th>
 </tr>
⚠️ No CHANGELOG changes made

Generated by 🚫 danger

@kzaher kzaher merged commit e730955 into ReactiveX:develop Jun 29, 2016
@tonyarnold tonyarnold deleted the fix/static-analysis-issues branch January 28, 2017 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants