[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov abandoned this revision.
ilya-biryukov added a comment.

Thanks for clarifying! I misread the standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157708

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

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

The prior behavior of Clang is correct. A search of a class scope is ill-formed 
if it finds an ambiguous result, see 
http://eel.is/c++draft/basic.lookup#class.member.lookup-6.sentence-2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157708

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

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a reviewer: clang-language-wg.
ilya-biryukov added a comment.

@shafik, @rsmith friendly ping for a review.

Also adding clang-language-wg for a wider set of potential reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157708

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

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:967
S.getScopeForContext(EqFD->getEnclosingNamespaceContext()));
   NonMembers.suppressAccessDiagnostics();
   for (NamedDecl *Op : NonMembers) {

I believe we should be able to expose the same errorenous behavior here, but I 
did not come up with a test yet, so did not change it yet.
We should update it and come up with a test before the change lands.

But I still wanted to get early feedback on the direction of this patch, so 
sending for review right away.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157708

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

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: shafik, rsmith.
Herald added a project: All.
ilya-biryukov requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The standard specified that Clang should 'search' for members in this
case, which does not involve full lookup and the corresponding checks.

See `[over.match.oper]p4` and `[basic.lookup.general]p3`.

The added test has an example code that breaks.
The change that exposed this behavior is 
cc1b6668c57170cd440d321037ced89d6a61a9cb 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157708

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/cxx20-reversed-operators-search.cpp


Index: clang/test/SemaCXX/cxx20-reversed-operators-search.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx20-reversed-operators-search.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// expected-no-diagnostics
+
+
+namespace members {
+
+struct X {
+  virtual ~X();
+  virtual bool operator ==(X);
+  bool operator !=(X);
+};
+
+struct Y {
+  virtual ~Y();
+  virtual bool operator ==(Y);
+  bool operator !=(Y);
+};
+
+struct Z : X, Y {
+  virtual bool operator==(Z);
+  bool operator==(X) override;
+  bool operator==(Y) override;
+};
+
+void test() {
+  bool b = Z() == Z();
+}
+
+} // namespace members
+
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -950,7 +950,10 @@
 LookupResult Members(S, NotEqOp, OpLoc,
  Sema::LookupNameKind::LookupMemberName);
 S.LookupQualifiedName(Members, RHSRec->getDecl());
-Members.suppressAccessDiagnostics();
+// According to [over.match.oper]p4 we should run "search" and not "lookup"
+// for reversed operators, so suppress diagnostics.
+Members.suppressDiagnostics();
+
 for (NamedDecl *Op : Members)
   if (FunctionsCorrespond(S.Context, EqFD, Op->getAsFunction()))
 return false;


Index: clang/test/SemaCXX/cxx20-reversed-operators-search.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx20-reversed-operators-search.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// expected-no-diagnostics
+
+
+namespace members {
+
+struct X {
+  virtual ~X();
+  virtual bool operator ==(X);
+  bool operator !=(X);
+};
+
+struct Y {
+  virtual ~Y();
+  virtual bool operator ==(Y);
+  bool operator !=(Y);
+};
+
+struct Z : X, Y {
+  virtual bool operator==(Z);
+  bool operator==(X) override;
+  bool operator==(Y) override;
+};
+
+void test() {
+  bool b = Z() == Z();
+}
+
+} // namespace members
+
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -950,7 +950,10 @@
 LookupResult Members(S, NotEqOp, OpLoc,
  Sema::LookupNameKind::LookupMemberName);
 S.LookupQualifiedName(Members, RHSRec->getDecl());
-Members.suppressAccessDiagnostics();
+// According to [over.match.oper]p4 we should run "search" and not "lookup"
+// for reversed operators, so suppress diagnostics.
+Members.suppressDiagnostics();
+
 for (NamedDecl *Op : Members)
   if (FunctionsCorrespond(S.Context, EqFD, Op->getAsFunction()))
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits