erichkeane wrote: >As I understood the author, more time was needed to prepare the tests and the >fix. I just tried to bring the tip of the tree to a good state soon (our >internal release process was blocked on this) without putting any pressure on >the author.
Typically when the author is engaging fairly quickly and only needs a short time (he had already identified the problem and only had process-related concerns), it is preferred to be understanding/polite and give them time to work on it. While I am sympathetic to your internal processes, it seems to me that they should be more tolerant of a ToT that is not particularly stable, as is the case with Clang. > With the standard as is, `B(A)` should be a better match because of > [[over.ics.list]p(7.1)](https://wg21.link/over.ics.list#7.1), it is an Exact > Match, and `B(std::vector<A>)` is a user defined conversion. > > With the current CWG2311 fix, the call to `B(A)` is an Exact Match because > it's an `A` prvalue. > > In either case, `B(A)` should unambiguously be chosen over > `B(std::vector<A>)` given `{ A() }` as an initializer. > > https://github.com/llvm/llvm-project/blob/7b11c08c664863fbcd7a3058179b0af3de5d28e4/clang/lib/Sema/SemaOverload.cpp#L1593 > > ^ This line is the issue: `isCopyConstructor()` checks only for lvalue > reference copy constructors, and the move constructor is selected (i.e., if > you force a copy constructor to be called instead, it works: > https://godbolt.org/z/rGEsPdMx8). > > There is no reason to check for copy constructors here. Removing > `Constructor->isCopyConstructor() &&` fixes the issue (`B(A)` is chosen, and > all existing tests still pass) > > @cor3ntin I have some extra test cases I would want to add too. Should I make > a new pull request? Or are we going to reopen this one and target llvm 19? If you haven't already, you can re-open this one. However, you'll ALSO have to make a 'differential' PR against 18.x so that this can be fixed there too. https://github.com/llvm/llvm-project/pull/77768 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
