[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D49718#1174281, @ahatanak wrote:

> Merge pushBlockObjectRelease and enterByrefCleanup.
>
> In https://reviews.llvm.org/D49718#1174113, @rjmccall wrote:
>
> > Heh, okay.  It looks like the runtime doesn't do anything different, but I think it's probably more correct to always pass `BLOCK_FIELD_IS_WEAK` when destroying a `__weak` block in GC modes.
>
>
> I tried passing `BLOCK_FIELD_IS_WEAK | BLOCK_FIELD_IS_BYREF` to the call to enterByrefCleanup in EmitAutoVarCleanups, but it looks like test/CodeGenObjC/blocks.m fails if I do so. The test was committed in r125823 and there is a comment that says " We're not supposed to pass BLOCK_FIELD_IS_WEAK here".
>
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110214/038996.html
>
> Should we change the check in the test?


Thanks for the archaeology; that's a very good question.  It looks to me like that comment was unrelated to the functionality change made by that patch, and nothing in the radar history explains it, either.  I don't see anything in the runtime source which suggests that it's wrong to pass `BYREF|WEAK` to `Block_object_dispose`.  Going way back, it does seem to have been necessary under GC to pass `BYREF|WEAK` for `Block_object_assign`, but the runtime never did anything special with the flag for `Block_object_dispose`; it just always treated it exactly the same as `BYREF` alone.

I think it's fine to just change the test.


Repository:
  rC Clang

https://reviews.llvm.org/D49718