[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

dewen added inline comments. Comment at: clang/test/CXX/class.derived/class.member.lookup/p11.cpp:15 +struct D: I1, I2, B2 { + using B1::f; + using B2::f; ``` bool correctness{true}; struct A { bool operator==(A const) const { return true; } };

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added a comment. In D155387#4579866 , @ilya-biryukov wrote: > I believe this should compile as according to (over.match.oper)p4 > : > >> A non-template function or function template F

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

ilya-biryukov added a comment. This change seems to expose a bug in Clang in C++20 see godbolt . I believe following code should compile, but it does not: struct X { virtual ~X(); virtual bool operator ==(X); bool operator !=(X); };

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

jyknight added a comment. In D155387#4557834 , @hctim wrote: > I found an issue with building Android using this patch. I've reduced it down > to the following problem where the evaluation of the `std::visit` is believed > to be non-exhaustive, but it

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

hctim added a comment. Hi folks, I found an issue with building Android using this patch. I've reduced it down to the following problem where the evaluation of the `std::visit` is believed to be non-exhaustive, but it seems like it is? Would you mind taking a look? Admittedly, my knowledge in

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcc1b6668c571: [Clang] Fix member lookup so that we dont ignore ambiguous lookups in some… (authored by shafik). Herald added a project: clang.

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik updated this revision to Diff 545283. shafik added a comment. Add release note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155387/new/ https://reviews.llvm.org/D155387 Files: clang/docs/ReleaseNotes.rst clang/include/clang/Sema/Lookup.h clang/lib/Sema/SemaOverload.cpp

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This looks good to me. I think we can further refine the handling of duplicate diagnostics but I don't think that needs to be done as part of this bugfix. Comment at:

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik updated this revision to Diff 544999. shafik added a comment. rebase CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155387/new/ https://reviews.llvm.org/D155387 Files: clang/include/clang/Sema/Lookup.h clang/lib/Sema/SemaOverload.cpp clang/lib/Sema/SemaTemplate.cpp

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik marked 4 inline comments as done. shafik added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:15191 OverloadCandidateSet::iterator Best; switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) { case OR_Success: rsmith wrote: >

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik updated this revision to Diff 544960. shafik marked an inline comment as done. shafik added a comment. - Updated so that we ignore ambiguous overload candidate if the name lookup was also ambiguous. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155387/new/

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:15191 OverloadCandidateSet::iterator Best; switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) { case OR_Success: shafik wrote: > @rsmith if `R.isAmbiguous()` should we

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:15191 OverloadCandidateSet::iterator Best; switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) { case OR_Success: @rsmith if `R.isAmbiguous()` should we even check the

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/include/clang/Sema/Lookup.h:197-200 +DiagnoseAccess(std::move(Other.DiagnoseAccess)), +DiagnoseAmbiguous(std::move(Other.DiagnoseAmbiguous)),

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik updated this revision to Diff 540866. shafik added a comment. - Adjusted more place to use `suppressAccessDiagnostics` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155387/new/ https://reviews.llvm.org/D155387 Files: clang/include/clang/Sema/Lookup.h

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:14930 LookupQualifiedName(R, Record->getDecl()); - R.suppressDiagnostics(); + R.suppressAccessDiagnostics(); shafik wrote: > rsmith wrote: > > shafik wrote: > > > I was a bit

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:14930 LookupQualifiedName(R, Record->getDecl()); - R.suppressDiagnostics(); + R.suppressAccessDiagnostics(); rsmith wrote: > shafik wrote: > > I was a bit conservative where I

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added inline comments. Comment at: clang/include/clang/Sema/Lookup.h:228-229 Other.Paths = nullptr; -Other.Diagnose = false; +Other.DiagnoseAccess = false; +Other.DiagnoseAmbiguous = false; return *this; cor3ntin wrote: > rsmith

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

cor3ntin added inline comments. Comment at: clang/include/clang/Sema/Lookup.h:228-229 Other.Paths = nullptr; -Other.Diagnose = false; +Other.DiagnoseAccess = false; +Other.DiagnoseAmbiguous = false; return *this; rsmith wrote: > cor3ntin

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

rsmith added inline comments. Comment at: clang/include/clang/Sema/Lookup.h:228-229 Other.Paths = nullptr; -Other.Diagnose = false; +Other.DiagnoseAccess = false; +Other.DiagnoseAmbiguous = false; return *this; cor3ntin wrote: > Does

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

cor3ntin added inline comments. Comment at: clang/include/clang/Sema/Lookup.h:197-200 +DiagnoseAccess(std::move(Other.DiagnoseAccess)), +DiagnoseAmbiguous(std::move(Other.DiagnoseAmbiguous)), AllowHidden(std::move(Other.AllowHidden)),

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik added a comment. It looks like the paragraphs in `class.member.lookup` have changed a lot and so the tests don't totally match in that directory. Fixing that seems like a big piece of work but maybe worth doing. Comment at: clang/include/clang/Sema/Lookup.h:152 +

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

shafik created this revision. shafik added reviewers: aaron.ballman, rsmith, cor3ntin. Herald added a project: All. shafik requested review of this revision. There are some cases during member lookup we are aggressively suppressing diagnostics when we should just be suppressing access control