[cfe-dev] Fwd: Code carefully copying LookupResults which aren't exactly copyable

[David Blaikie via cfe-dev]

On Wed, Aug 19, 2015 at 5:19 PM, Kaelyn Takata <rikka at google.com> wrote:

> As I recall, the capture of the LookupResult (which was believed to be
> safe) was a convenient way of capturing the lookup-parameter state of the
> LookupResult--the identifier being looked up, it's location, the lookup
> kind, etc--
>

I wonder if we could make a little struct of that state and pull it out of
the original LookupResult, capture it, and put it back into a LookupResult
on the other side...


> instead of breaking them out into a bunch of separate local variables to
> be captured by the lambda and used to rebuild a LookupResult suitable for
> passing into SemaRef.BuildMemberReferenceExpr (which IIRC depends on the
> LookupResult telling it whether overload resolution is necessary).
>
> On Tue, Aug 18, 2015 at 8:52 PM, David Blaikie via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Perhaps I should remove the old lists from my contacts...
>>
>> ---------- Forwarded message ----------
>> From: David Blaikie <dblaikie at gmail.com>
>> Date: Tue, Aug 18, 2015 at 8:39 PM
>> Subject: Code carefully copying LookupResults which aren't exactly
>> copyable
>> To: Richard Smith <richard at metafoo.co.uk>, Kaelyn Takata <
>> rikka at google.com>, cfe-dev Developers <cfe-dev at cs.uiuc.edu>
>>
>>
>> More -Wdeprecated cleanup.
>>
>> Turns out LookupResults are copied in at least one place:
>>
>> SemaExprMember.cpp:651
>>
>> Essentially this function takes a LookupResult, captures it by value into
>> a lambda, within the body of the lambda it suppresses diagnostics on the
>> LookupResult and then does some other stuff.
>>
>> LookupResult's dtor does a couple of things - it emits diagnostics, if
>> needed. And it destroys the CXXBasePaths in the LookupResult. As
>> luck/design would have it, the LookupResult that is copied here never has a
>> non-null CXXBasePaths and, as mentioned, always disables diagnostics (if
>> the lambda is actually called - if it is not, maybe bad things could
>> happen? It could emit diagnostics unintentionally?).
>>
>> Having LookupResult be copyable in general seems problematic - if extra
>> care is not taken it could easily double delete and/or emit excess
>> diagnostics.
>>
>> Any ideas how this might be improved?
>>
>> Honestly I seem to recall being involved in the code review, and maybe I
>> even suggested this approach, not knowing what monsters lurked beneath.
>> Maybe the original code ferried just the right LookupResult parameters
>> across the capture boundary? That would be safe & then we could make
>> LookupResult non-copyable again. (& can unique_ptr the CXXBasePaths (or
>> even Optional<CXXBasePaths>) eventually)
>>
>> - Dave
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150819/2c095be2/attachment.html>