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

Fix clang errors #378

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix clang errors #378

wants to merge 1 commit into from

Conversation

davidrothera
Copy link

Changes

  • Add resignFirstResponder calls to the superclass
  • Change self.fields to [self fields] which allows the array to be initiated if it isn't already

Fixes

Fixes #365

Testing

  • Ran unit tests, no failures
  • Ran clang analyze again, no issues

* Add `resignFirstResponder` calls to the superclass
* Change `self.fields` to `[self fields]` which allows the array to be initiated if it isn't already
@nicklockwood
Copy link
Owner

That doesn't make a whole lot of sense. self.fields is functionally equivalent to [self fields]. Can you explain what effect you believe this change had on the behavior?

@davidrothera
Copy link
Author

The basis behind my change was that calling self.fields accesses the instance variable which could be nil however using the [self fields] method[1] will instantiate a non-existing instance variable.

[1] https://github.com/davidrothera/FXForms/blob/develop/FXForms/FXForms.m#L1732

@nicklockwood
Copy link
Owner

But self.fields doesn't access the instance variable. It calls the - (NSMutableArray *)fields method. To access the instance variable, you'd have to write self->_fields.

Try putting a breakpoint inside - (NSMutableArray *)fields and you can verify that it's called when you access self.fields.

I think perhaps the issue that you're referring to is "Argument to 'NSMutableArray' method 'addObject;:' cannot be nil"? In which case the issue it's complaining about is that the field value being added to the array is nil, not the array itself.

I'll fix this in the next update.

@davidrothera
Copy link
Author

Yeah I think you are right, its strange though as making the change in the PR resolved the clang error and retained all functionality.

@nicklockwood
Copy link
Owner

Hmm, I can't actually replicate a clang warning on the line you fixed. The warning I'm seeing is on the line a few below that: [self.fields addObject:field]

@davidrothera
Copy link
Author

Yes, the error was on that line and the change I made resolved that one lower down.

@nicklockwood
Copy link
Owner

I think maybe the change above just confused the analyzer so that it couldn't parse the logic below anymore ¯_(ツ)_/¯

@davidrothera
Copy link
Author

Quite possibly, clang isn't the smartest ;)

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.

None yet

2 participants