jansvoboda11 added a comment.

> It doesn't look like the patch handles `volatile _Atomic` correctly, though.

That should be fixed in the new revision.

> I know we do a lot of candidate prefiltering, and that that can be difficult 
> because of uesr-defined conversions.  How do those things interact with 
> qualifiers?

I don't know.

> Like, I notice the existing code is adding both `(T &, T)` and `(volatile T 
> &, T)` when the LHS is `volatile`; is there a reason that's necessary?

Looking at Git history, that was always the case. I don't see any particular 
reason we do that eagerly.

> Because we can't actually convert the LHS to remove those qualifiers, and 
> adding combinatoric candidates could get very expensive if we start doing it 
> for arbitrary extended qualifiers, which it feels like we ought to.  (You 
> could certainly have e.g. a `volatile _Atomic __myaddrspace` l-value.)  If 
> this is only necessary when the LHS has a user-defined conversion, then maybe 
> we could at least not do it in the normal case just because the RHS is an 
> overloaded type.

Yup, seems like we could be more lazy/minimal about that. I can look into that 
as a follow-up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125349/new/

https://reviews.llvm.org/D125349

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to