Fix/694 static analyser issues #1198
Conversation
…y ARTQueueAdditions category + some formatting issues
+ removed obsolete art_nullable
…er constructor declared with NS_ASSUME_NONNULL_BEGIN)
- nullable specifiers for methods that actually can return null (***From***); - removed unnecessary checks for methods that can't return null (***To***);
…y, because id declared as nullable
…h was obviously a typo
maratal
requested review from
QuintinWillison,
lukasz-szyszkowski and
lawrence-forooghian
… return nil because of internal error
…il as copy from argument
…ve nil as create from argument
…hQueues constructor has nullable userQueue argument
- nullable for getters for corresponding nullable properties - removed unnecessary nil check
…ross the library. Xcode static analyser issue fix: nullable for `observer` property since it needs to be released.
|
Hey @maratal – it looks like you're still working on this one (new commits pushed yesterday). Perhaps it would be best to convert it to a draft so that we know when it's truly ready for review? Because I see that @lukasz-szyszkowski has already approved it, but it's changed since he approved it. |
|
I'm also curious where in Xcode you're seeing these static analyser warnings? |
It certainly would, but I've finished already =) Now it's ready.
Menu->Product->Analyze |
There was a problem hiding this comment.
As discussed in yesterday's in-person call, I would like to see evidence that this is being (or will be) checked in the CI environment (GitHub workflows). I believe that would be one of two options:
- Add to this PR a commit that switches on static analyzer when building (
RUN_CLANG_STATIC_ANALYZERbuild setting, I think) - or, perhaps, runxcodebuild analyzefrom workflow(s) - Create an issue to add this to our backlog - recognising that when we work on that there may be additional transgressions to fix
There was a problem hiding this comment.
Looks good – I agree with what @QuintinWillison said about getting this done in CI, too.
|
I agree too, but I think it should be done in a separate issue. Will do. |
Fixed the most obvious static analyser issues