Upgrade xcode project to latest Recommended Settings #1280
Conversation
|
@swift-ci please test |
| @@ -496,8 +496,8 @@ static CFMessagePortRef __CFMessagePortCreateRemote(CFAllocatorRef allocator, CF | |||
| ctx.retain = NULL; | |||
| ctx.release = NULL; | |||
| ctx.copyDescription = NULL; | |||
| task_get_bootstrap_port(mach_task_self(), &bp); | |||
| native = (KERN_SUCCESS == ret) ? CFMachPortCreateWithPort(allocator, port, __CFMessagePortDummyCallback, &ctx, NULL) : NULL; | |||
| ret = task_get_bootstrap_port(mach_task_self(), &bp); | |||
There was a problem hiding this comment.
These couple of lines may need a quick eyeball by @phausler -- the direct context in the GitHub view doesn't necessarily determine that these changes are valid. What was the warning that you were trying to fix here? Unused return or local variable?
There was a problem hiding this comment.
So looking at the changes they look sensible in the context of this change, but it would be good to see if the fixes can be upstreamed efficiently.
There was a problem hiding this comment.
I think they were both reads before being initialised, but the port -> bp fix is more of an educated guess :)
There was a problem hiding this comment.
I don't think this is what you want actually. This function really shouldn't exist at all in this environment.
The core idea here is to look up the port by name but that part is missing. We should probably just replace the entire thing with return NULL.
There was a problem hiding this comment.
If we want to keep the rest of the code for some reason (documentation purposes?) then we should not bother with task_get_bootstrap_port and just make sure port == NULL for the rest of the function.
There was a problem hiding this comment.
If the function can be replaced with a return NULL then it would be better to just do that. The old body will still be in git for the curious.
spevans
force-pushed
the
pr_upgrade_xcode_project
branch
from
0b97102 to
e982642
Compare
|
I removed the |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
| @@ -2763,7 +2775,6 @@ | |||
| "-DCF_CHARACTERSET_UNICODE_DATA_B=\\\\\"CoreFoundation/CharacterSets/CFUnicodeData-B.mapping\\\\\"", | |||
| "-DCF_CHARACTERSET_UNICHAR_DB=\\\\\"CoreFoundation/CharacterSets/CFUniCharPropertyDatabase.data\\\\\"", | |||
| "-DCF_CHARACTERSET_BITMAP=\\\\\"CoreFoundation/CharacterSets/CFCharacterSetBitmaps.bitmap\\\\\"", | |||
| "-Wno-nullability-completeness-on-arrays", | |||
There was a problem hiding this comment.
Was this removed by the upgrade process? Is it implied by default now?
There was a problem hiding this comment.
No, I removed it manually. It was added in #988 but
a) I think its a useful warning to have,
b) The build.ninja doesnt have this flag so these warnings currently show up on Linux anyway and so this keeps the two builds in sync.
|
Given that Xcode 9.1 has been released, can you see if there are other changes which get added as part of that, and resubmit a newer version? |
|
@alblue Apart from bumping the LastUpgradeCheck from |
|
@alblue Any other issues with this? |
|
Should we change the P.S. 👍 to this PR. |
|
I'll update to the latest and rebase just to tick that box then we can merge it |
- This enables more clang warnings which finds some issues in the CoreFoundation code. Fix some of these.
|
Sounds good. |
spevans
force-pushed
the
pr_upgrade_xcode_project
branch
from
e982642 to
86bd361
Compare
|
@alblue I bumped the |
|
Works for me. |
in the CoreFoundation code. Fix some of these.