Fwd: Code carefully copying LookupResults which aren't exactly copyable

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Fwd: Code carefully copying LookupResults which aren't exactly copyable

Ilya Biryukov via cfe-dev
Perhaps I should remove the old lists from my contacts...
---------- Forwarded message ----------
From: David Blaikie <[hidden email]>
Date: Tue, Aug 18, 2015 at 8:39 PM
Subject: Code carefully copying LookupResults which aren't exactly copyable
To: Richard Smith <[hidden email]>, Kaelyn Takata <[hidden email]>, cfe-dev Developers <[hidden email]>


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Fwd: Code carefully copying LookupResults which aren't exactly copyable

Ilya Biryukov via cfe-dev
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--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 <[hidden email]> wrote:
Perhaps I should remove the old lists from my contacts...

---------- Forwarded message ----------
From: David Blaikie <[hidden email]>
Date: Tue, Aug 18, 2015 at 8:39 PM
Subject: Code carefully copying LookupResults which aren't exactly copyable
To: Richard Smith <[hidden email]>, Kaelyn Takata <[hidden email]>, cfe-dev Developers <[hidden email]>


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Fwd: Code carefully copying LookupResults which aren't exactly copyable

Ilya Biryukov via cfe-dev


On Wed, Aug 19, 2015 at 5:19 PM, Kaelyn Takata <[hidden email]> 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 <[hidden email]> wrote:
Perhaps I should remove the old lists from my contacts...

---------- Forwarded message ----------
From: David Blaikie <[hidden email]>
Date: Tue, Aug 18, 2015 at 8:39 PM
Subject: Code carefully copying LookupResults which aren't exactly copyable
To: Richard Smith <[hidden email]>, Kaelyn Takata <[hidden email]>, cfe-dev Developers <[hidden email]>


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev