Fixed the last 6 warnings, enabled "Treat warnings as errors"#29
Fixed the last 6 warnings, enabled "Treat warnings as errors"#29cristiankocza-sv wants to merge 1 commit intoVoxer:Voxerfrom
Conversation
cristiankocza-sv
commented
Oct 1, 2015
cristiankocza-sv
commented
- Fixed incompatible code property of ISpdyError with the base class NSError: replaced the getter declaration by a property one, using NS_ENUM to make ISpdyErrorCode compatible with NSInteger (the type of NSError's code property), and updated a format string due to this change (common.m:185)
- Fixed _closeSocket declaration and implementation differing in return type
- Fixed an integer precision warning (ispdy.m:1047)
- Fixed the null argument passed to non-null parameter warning in loop.m, added for this a dummy timer method and passing that to the timer
- Fixed retain cycle warning by using weak reference to self in request.m
- Updated the project file and the podspec to enable the "Treat warnings as errors" build setting
- Fixed incompatible code property of ISpdyError with the base class NSError: replaced the getter declaration by a property one, using NS_ENUM to make ISpdyErrorCode compatible with NSInteger (the type of NSError's code property), and updated a format string due to this change (common.m:185) - Fixed _closeSocket declaration and implementation differing in return type - Fixed an integer precision warning (ispdy.m:1047) - Fixed the null argument passed to non-null parameter warning in loop.m, added for this a dummy timer method and passing that to the timer - Fixed retain cycle warning by using weak reference to self in request.m - Updated the project file and the podspec to enable the "Treat warnings as errors" build setting
|
@indutny, @georgekola please take a look at this PR, I recommend we fix all warnings and enable the "treat warnings as error" build setting |
There was a problem hiding this comment.
This could be a bit dangerous. If the request will loose references - the data will be lost.
There was a problem hiding this comment.
If the request gets deallocated then there's no point in updating it's window anyway, right? Unless we want to keep the request alive until this block gets executed...
There was a problem hiding this comment.
@cristiankocza _windowUpdate is a way to unqueue accumulated data from request. Using weak here will probably cause some data to be not delivered...
There was a problem hiding this comment.
Does this mean that a final _updateWindow message will be passed to the ISpdyRequest object? The one that nils the window_out_queue_ variable, thus breaking the circular reference? If yes, this means the request has at least one reference held by the caller of _updateWindow. If not, it means that the last _updateWindow that enqueues the block in discussion will create a leak.
There was a problem hiding this comment.
The above assumptions are based only by looking at the code from _updateWindow, as I'm not familiar with the ISpdy design pardon any naive assumptions :)
There was a problem hiding this comment.
That's the thing. This message will happen sometime in a future, when the server will acknowledge previously sent data. It will unlikely coincide with the refcount == 0.
There was a problem hiding this comment.
But until then, won't a reference to the request be held somewhere, to know on which object to call _updateWindow on?
There was a problem hiding this comment.
If we need a self-retaining logic for ISpdyRequest we might keep a reference to itself, and nullify it when appropriate.
There was a problem hiding this comment.
Hm... this actually should work. Thanks!
2 participants
