<div dir="ltr">Thanks! Re-landed in r362924.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jun 7, 2019 at 9:15 PM Vlad Tsyrklevich via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">This change caused LSan failures and has been reverted in r362830: <a href="http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/32809" target="_blank">http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/32809</a></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jun 7, 2019 at 2:42 AM Sam McCall via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: sammccall<br>
Date: Fri Jun  7 02:45:17 2019<br>
New Revision: 362785<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=362785&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=362785&view=rev</a><br>
Log:<br>
[CodeComplete] Improve overload handling for C++ qualified and ref-qualified methods.<br>
<br>
Summary:<br>
- when a method is not available because of the target value kind (e.g. an &&<br>
  method on a Foo& variable), then don't offer it.<br>
- when a method is effectively shadowed by another method from the same class<br>
  with a) an identical argument list and b) superior qualifiers, then don't<br>
  offer it.<br>
<br>
Reviewers: ilya-biryukov<br>
<br>
Subscribers: cfe-commits<br>
<br>
Tags: #clang<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D62582" rel="noreferrer" target="_blank">https://reviews.llvm.org/D62582</a><br>
<br>
Modified:<br>
    cfe/trunk/lib/Sema/SemaCodeComplete.cpp<br>
    cfe/trunk/test/CodeCompletion/member-access.cpp<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaCodeComplete.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=362785&r1=362784&r2=362785&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=362785&r1=362784&r2=362785&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp Fri Jun  7 02:45:17 2019<br>
@@ -16,7 +16,9 @@<br>
 #include "clang/AST/ExprCXX.h"<br>
 #include "clang/AST/ExprObjC.h"<br>
 #include "clang/AST/QualTypeNames.h"<br>
+#include "clang/AST/Type.h"<br>
 #include "clang/Basic/CharInfo.h"<br>
+#include "clang/Basic/Specifiers.h"<br>
 #include "clang/Lex/HeaderSearch.h"<br>
 #include "clang/Lex/MacroInfo.h"<br>
 #include "clang/Lex/Preprocessor.h"<br>
@@ -152,9 +154,16 @@ private:<br>
   /// different levels of, e.g., the inheritance hierarchy.<br>
   std::list<ShadowMap> ShadowMaps;<br>
<br>
+  /// Overloaded C++ member functions found by SemaLookup.<br>
+  /// Used to determine when one overload is dominated by another.<br>
+  llvm::DenseMap<std::pair<DeclContext *, /*Name*/uintptr_t>, ShadowMapEntry><br>
+      OverloadMap;<br>
+<br>
   /// If we're potentially referring to a C++ member function, the set<br>
   /// of qualifiers applied to the object type.<br>
   Qualifiers ObjectTypeQualifiers;<br>
+  /// The kind of the object expression, for rvalue/lvalue overloads.<br>
+  ExprValueKind ObjectKind;<br>
<br>
   /// Whether the \p ObjectTypeQualifiers field is active.<br>
   bool HasObjectTypeQualifiers;<br>
@@ -230,8 +239,9 @@ public:<br>
   /// out member functions that aren't available (because there will be a<br>
   /// cv-qualifier mismatch) or prefer functions with an exact qualifier<br>
   /// match.<br>
-  void setObjectTypeQualifiers(Qualifiers Quals) {<br>
+  void setObjectTypeQualifiers(Qualifiers Quals, ExprValueKind Kind) {<br>
     ObjectTypeQualifiers = Quals;<br>
+    ObjectKind = Kind;<br>
     HasObjectTypeQualifiers = true;<br>
   }<br>
<br>
@@ -1157,6 +1167,53 @@ static void setInBaseClass(ResultBuilder<br>
   R.InBaseClass = true;<br>
 }<br>
<br>
+enum class OverloadCompare { BothViable, Dominates, Dominated };<br>
+// Will Candidate ever be called on the object, when overloaded with Incumbent?<br>
+// Returns Dominates if Candidate is always called, Dominated if Incumbent is<br>
+// always called, BothViable if either may be called dependending on arguments.<br>
+// Precondition: must actually be overloads!<br>
+static OverloadCompare compareOverloads(const CXXMethodDecl &Candidate,<br>
+                                        const CXXMethodDecl &Incumbent,<br>
+                                        const Qualifiers &ObjectQuals,<br>
+                                        ExprValueKind ObjectKind) {<br>
+  if (Candidate.isVariadic() != Incumbent.isVariadic() ||<br>
+      Candidate.getNumParams() != Incumbent.getNumParams() ||<br>
+      Candidate.getMinRequiredArguments() !=<br>
+          Incumbent.getMinRequiredArguments())<br>
+    return OverloadCompare::BothViable;<br>
+  for (unsigned I = 0, E = Candidate.getNumParams(); I != E; ++I)<br>
+    if (Candidate.parameters()[I]->getType().getCanonicalType() !=<br>
+        Incumbent.parameters()[I]->getType().getCanonicalType())<br>
+      return OverloadCompare::BothViable;<br>
+  if (!llvm::empty(Candidate.specific_attrs<EnableIfAttr>()) ||<br>
+      !llvm::empty(Incumbent.specific_attrs<EnableIfAttr>()))<br>
+    return OverloadCompare::BothViable;<br>
+  // At this point, we know calls can't pick one or the other based on<br>
+  // arguments, so one of the two must win. (Or both fail, handled elsewhere).<br>
+  RefQualifierKind CandidateRef = Candidate.getRefQualifier();<br>
+  RefQualifierKind IncumbentRef = Incumbent.getRefQualifier();<br>
+  if (CandidateRef != IncumbentRef) {<br>
+    // If the object kind is LValue/RValue, there's one acceptable ref-qualifier<br>
+    // and it can't be mixed with ref-unqualified overloads (in valid code).<br>
+<br>
+    // For xvalue objects, we prefer the rvalue overload even if we have to<br>
+    // add qualifiers (which is rare, because const&& is rare).<br>
+    if (ObjectKind == clang::VK_XValue)<br>
+      return CandidateRef == RQ_RValue ? OverloadCompare::Dominates<br>
+                                       : OverloadCompare::Dominated;<br>
+  }<br>
+  // Now the ref qualifiers are the same (or we're in some invalid state).<br>
+  // So make some decision based on the qualifiers.<br>
+  Qualifiers CandidateQual = Candidate.getMethodQualifiers();<br>
+  Qualifiers IncumbentQual = Incumbent.getMethodQualifiers();<br>
+  bool CandidateSuperset = CandidateQual.compatiblyIncludes(IncumbentQual);<br>
+  bool IncumbentSuperset = IncumbentQual.compatiblyIncludes(CandidateQual);<br>
+  if (CandidateSuperset == IncumbentSuperset)<br>
+    return OverloadCompare::BothViable;<br>
+  return IncumbentSuperset ? OverloadCompare::Dominates<br>
+                           : OverloadCompare::Dominated;<br>
+}<br>
+<br>
 void ResultBuilder::AddResult(Result R, DeclContext *CurContext,<br>
                               NamedDecl *Hiding, bool InBaseClass = false) {<br>
   if (R.Kind != Result::RK_Declaration) {<br>
@@ -1233,6 +1290,44 @@ void ResultBuilder::AddResult(Result R,<br>
           // qualifiers.<br>
           return;<br>
         }<br>
+        // Detect cases where a ref-qualified method cannot be invoked.<br>
+        switch (Method->getRefQualifier()) {<br>
+          case RQ_LValue:<br>
+            if (ObjectKind != VK_LValue && !MethodQuals.hasConst())<br>
+              return;<br>
+            break;<br>
+          case RQ_RValue:<br>
+            if (ObjectKind == VK_LValue)<br>
+              return;<br>
+            break;<br>
+          case RQ_None:<br>
+            break;<br>
+        }<br>
+<br>
+        /// Check whether this dominates another overloaded method, which should<br>
+        /// be suppressed (or vice versa).<br>
+        /// Motivating case is const_iterator begin() const vs iterator begin().<br>
+        auto &OverloadSet = OverloadMap[std::make_pair(<br>
+            CurContext, Method->getDeclName().getAsOpaqueInteger())];<br>
+        for (const DeclIndexPair& Entry : OverloadSet) {<br>
+          Result &Incumbent = Results[Entry.second];<br>
+          switch (compareOverloads(*Method,<br>
+                                   *cast<CXXMethodDecl>(Incumbent.Declaration),<br>
+                                   ObjectTypeQualifiers, ObjectKind)) {<br>
+          case OverloadCompare::Dominates:<br>
+            // Replace the dominated overload with this one.<br>
+            // FIXME: if the overload dominates multiple incumbents then we<br>
+            // should remove all. But two overloads is by far the common case.<br>
+            Incumbent = std::move(R);<br>
+            return;<br>
+          case OverloadCompare::Dominated:<br>
+            // This overload can't be called, drop it.<br>
+            return;<br>
+          case OverloadCompare::BothViable:<br>
+            break;<br>
+          }<br>
+        }<br>
+        OverloadSet.Add(Method, Results.size());<br>
       }<br>
<br>
   // Insert this result into the set of results.<br>
@@ -3997,7 +4092,8 @@ void Sema::CodeCompleteOrdinaryName(Scop<br>
   // the member function to filter/prioritize the results list.<br>
   auto ThisType = getCurrentThisType();<br>
   if (!ThisType.isNull())<br>
-    Results.setObjectTypeQualifiers(ThisType->getPointeeType().getQualifiers());<br>
+    Results.setObjectTypeQualifiers(ThisType->getPointeeType().getQualifiers(),<br>
+                                    VK_LValue);<br>
<br>
   CodeCompletionDeclConsumer Consumer(Results, CurContext);<br>
   LookupVisibleDecls(S, LookupOrdinaryName, Consumer,<br>
@@ -4551,13 +4647,12 @@ AddObjCProperties(const CodeCompletionCo<br>
   }<br>
 }<br>
<br>
-static void<br>
-AddRecordMembersCompletionResults(Sema &SemaRef, ResultBuilder &Results,<br>
-                                  Scope *S, QualType BaseType, RecordDecl *RD,<br>
-                                  Optional<FixItHint> AccessOpFixIt) {<br>
+static void AddRecordMembersCompletionResults(<br>
+    Sema &SemaRef, ResultBuilder &Results, Scope *S, QualType BaseType,<br>
+    ExprValueKind BaseKind, RecordDecl *RD, Optional<FixItHint> AccessOpFixIt) {<br>
   // Indicate that we are performing a member access, and the cv-qualifiers<br>
   // for the base object type.<br>
-  Results.setObjectTypeQualifiers(BaseType.getQualifiers());<br>
+  Results.setObjectTypeQualifiers(BaseType.getQualifiers(), BaseKind);<br>
<br>
   // Access to a C/C++ class, struct, or union.<br>
   Results.allowNestedNameSpecifiers();<br>
@@ -4638,18 +4733,20 @@ void Sema::CodeCompleteMemberReferenceEx<br>
     Base = ConvertedBase.get();<br>
<br>
     QualType BaseType = Base->getType();<br>
+    ExprValueKind BaseKind = Base->getValueKind();<br>
<br>
     if (IsArrow) {<br>
-      if (const PointerType *Ptr = BaseType->getAs<PointerType>())<br>
+      if (const PointerType *Ptr = BaseType->getAs<PointerType>()) {<br>
         BaseType = Ptr->getPointeeType();<br>
-      else if (BaseType->isObjCObjectPointerType())<br>
+        BaseKind = VK_LValue;<br>
+      } else if (BaseType->isObjCObjectPointerType())<br>
         /*Do nothing*/;<br>
       else<br>
         return false;<br>
     }<br>
<br>
     if (const RecordType *Record = BaseType->getAs<RecordType>()) {<br>
-      AddRecordMembersCompletionResults(*this, Results, S, BaseType,<br>
+      AddRecordMembersCompletionResults(*this, Results, S, BaseType, BaseKind,<br>
                                         Record->getDecl(),<br>
                                         std::move(AccessOpFixIt));<br>
     } else if (const auto *TST =<br>
@@ -4658,13 +4755,13 @@ void Sema::CodeCompleteMemberReferenceEx<br>
       if (const auto *TD =<br>
               dyn_cast_or_null<ClassTemplateDecl>(TN.getAsTemplateDecl())) {<br>
         CXXRecordDecl *RD = TD->getTemplatedDecl();<br>
-        AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD,<br>
-                                          std::move(AccessOpFixIt));<br>
+        AddRecordMembersCompletionResults(*this, Results, S, BaseType, BaseKind,<br>
+                                          RD, std::move(AccessOpFixIt));<br>
       }<br>
     } else if (const auto *ICNT = BaseType->getAs<InjectedClassNameType>()) {<br>
       if (auto *RD = ICNT->getDecl())<br>
-        AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD,<br>
-                                          std::move(AccessOpFixIt));<br>
+        AddRecordMembersCompletionResults(*this, Results, S, BaseType, BaseKind,<br>
+                                          RD, std::move(AccessOpFixIt));<br>
     } else if (!IsArrow && BaseType->isObjCObjectPointerType()) {<br>
       // Objective-C property reference.<br>
       AddedPropertiesSet AddedProperties;<br>
<br>
Modified: cfe/trunk/test/CodeCompletion/member-access.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeCompletion/member-access.cpp?rev=362785&r1=362784&r2=362785&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeCompletion/member-access.cpp?rev=362785&r1=362784&r2=362785&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeCompletion/member-access.cpp (original)<br>
+++ cfe/trunk/test/CodeCompletion/member-access.cpp Fri Jun  7 02:45:17 2019<br>
@@ -210,3 +210,66 @@ void test3(const Proxy2 &p) {<br>
 // CHECK-CC9: memfun2 (InBase) : [#void#][#Base3::#]memfun2(<#int#>) (requires fix-it: {181:4-181:5} to "->")<br>
 // CHECK-CC9: memfun3 : [#int#]memfun3(<#int#>) (requires fix-it: {181:4-181:5} to "->")<br>
 // CHECK-CC9: operator-> : [#Derived *#]operator->()[# const#]<br>
+<br>
+// These overload sets differ only by return type and this-qualifiers.<br>
+// So for any given callsite, only one is available.<br>
+struct Overloads {<br>
+  double ConstOverload(char);<br>
+  int ConstOverload(char) const;<br>
+<br>
+  int RefOverload(char) &;<br>
+  double RefOverload(char) const&;<br>
+  char RefOverload(char) &&;<br>
+};<br>
+void testLValue(Overloads& Ref) {<br>
+  Ref.<br>
+}<br>
+void testConstLValue(const Overloads& ConstRef) {<br>
+  ConstRef.<br>
+}<br>
+void testRValue() {<br>
+  Overloads().<br>
+}<br>
+void testXValue(Overloads& X) {<br>
+  static_cast<Overloads&&>(X).<br>
+}<br>
+<br>
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:225:7 %s -o - | FileCheck -check-prefix=CHECK-LVALUE %s \<br>
+// RUN: --implicit-check-not="[#int#]ConstOverload(" \<br>
+// RUN: --implicit-check-not="[#double#]RefOverload(" \<br>
+// RUN: --implicit-check-not="[#char#]RefOverload("<br>
+// CHECK-LVALUE-DAG: [#double#]ConstOverload(<br>
+// CHECK-LVALUE-DAG: [#int#]RefOverload(<br>
+<br>
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:228:12 %s -o - | FileCheck -check-prefix=CHECK-CONSTLVALUE %s \<br>
+// RUN: --implicit-check-not="[#double#]ConstOverload(" \<br>
+// RUN: --implicit-check-not="[#int#]RefOverload(" \<br>
+// RUN: --implicit-check-not="[#char#]RefOverload("<br>
+// CHECK-CONSTLVALUE: [#int#]ConstOverload(<br>
+// CHECK-CONSTLVALUE: [#double#]RefOverload(<br>
+<br>
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:231:15 %s -o - | FileCheck -check-prefix=CHECK-PRVALUE %s \<br>
+// RUN: --implicit-check-not="[#int#]ConstOverload(" \<br>
+// RUN: --implicit-check-not="[#int#]RefOverload(" \<br>
+// RUN: --implicit-check-not="[#double#]RefOverload("<br>
+// CHECK-PRVALUE: [#double#]ConstOverload(<br>
+// CHECK-PRVALUE: [#char#]RefOverload(<br>
+<br>
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:234:31 %s -o - | FileCheck -check-prefix=CHECK-XVALUE %s \<br>
+// RUN: --implicit-check-not="[#int#]ConstOverload(" \<br>
+// RUN: --implicit-check-not="[#int#]RefOverload(" \<br>
+// RUN: --implicit-check-not="[#double#]RefOverload("<br>
+// CHECK-XVALUE: [#double#]ConstOverload(<br>
+// CHECK-XVALUE: [#char#]RefOverload(<br>
+<br>
+void testOverloadOperator() {<br>
+  struct S {<br>
+    char operator=(int) const;<br>
+    int operator=(int);<br>
+  } s;<br>
+  return s.<br>
+}<br>
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:270:12 %s -o - | FileCheck -check-prefix=CHECK-OPER %s \<br>
+// RUN: --implicit-check-not="[#char#]operator=("<br>
+// CHECK-OPER: [#int#]operator=(<br>
+<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>