r224329 - Renamed RefersToEnclosingLocal bitfield to RefersToCapturedVariable.

[John McCall]

> On Jan 4, 2015, at 1:11 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Sat, Jan 3, 2015 at 1:46 PM, John McCall <rjmccall at apple.com <mailto:rjmccall at apple.com>> wrote:
>> On Jan 2, 2015, at 7:10 AM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>> On Tue, Dec 16, 2014 at 12:01 AM, Alexey Bataev <a.bataev at hotmail.com <mailto:a.bataev at hotmail.com>> wrote:
>> Author: abataev
>> Date: Tue Dec 16 02:01:48 2014
>> New Revision: 224329
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=224329&view=rev <http://llvm.org/viewvc/llvm-project?rev=224329&view=rev>
>> Log:
>> Renamed RefersToEnclosingLocal bitfield to RefersToCapturedVariable.
>> Bitfield RefersToEnclosingLocal of Stmt::DeclRefExprBitfields renamed to RefersToCapturedVariable to reflect latest changes introduced in commit 224323. Also renamed method Expr::refersToEnclosingLocal() to Expr::refersToCapturedVariable() and comments for constant arguments.
>> No functional changes.
>> 
>> This seems like a bad idea for me. It's incorrect: for a lambda, the flag means that the DeclRefExpr refers to an enclosing local, and does *not* imply that the variable is necessarily captured. This confusion has already led to a bug (fixed in r225060).
> 
> If that’s actually a useful property to track, I have no complaint about tracking the two things separately.  I don’t think DRE is short of bits.
> 
> The problem is that we can't reasonably track whether a DRE refers to a captured variable, because that is not a local property of the DRE; it depends on non-trivial properties of the context in which the DRE is used (and more generally it depends on how else that same variable is used elsewhere in the lambda, but we can largely ignore that).
> 
> I'd be fine having separate flags for 'refers to enclosing local' and 'refers to captured global' (if the latter is the point of this change), but we should not claim a DRE refers to a capture when it does not.

Richard, I understand that not all DREs are potentially evaluated and thus may not actually induce capturing, and I concede your point that the flag's name shouldn’t use the word “capture” if it’s set on unevaluated DREs.  The question is this: what information do we actually need to track on DREs?

It is very useful for IR-gen to be able to immediately know whether a DRE refers to a captured local or global variable, no matter why that capture was performed.  As far as I’m aware, IR-gen does not care about whether the flag is set on unevaluated references to enclosing variables, because IR-gen should never see an unevaluated use in normal expression emission.

I do not know if there is any reason to track whether a DRE refers to an enclosing local, independently of whether that enclosing local was captured.  If you think there is, then we will need a bit for it.  If it’s important to track that but *not* include captured globals, we will need separate bits; otherwise, since the difference only seems to exist on unevaluated DREs, and that doesn’t conflict with IR-gen’s needs, we can use the same bit.

If we use the same bit, and the bit shouldn’t be set on unevaluated references, we will need to avoid using the word “capture", but we should also avoid using the word “local”.  Probably we should just invent our own bit of jargon.

If you don’t actually care about tracking unevaluated references to enclosing locals in the AST, and are just making sure that the name isn’t actively misleading, then I suggest we name the bit “RefersToCapturedVariable" and simply say that it’s never set on unevaluated references.  I think it’s a reasonable gloss to say that unevaluated references always semantically refer to the original variable even if it’s otherwise captured.  We can handle PPE contexts by just setting the bit retroactively as part of the same walk that finds ODR uses.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150104/71de2776/attachment.html>