Addressing code review comments for commit 135509 - Add FixItHints in case a C++ function call is missing * or & operators on

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@135643 91177308-0d34-0410-b5e6-96231b3b80d8

4 files changed, 58 insertions, 13 deletions

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index dad311ceb4..2b96e0bef1 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1594,7 +1594,9 @@ def note_ovl_candidate_bad_conv : Note<"candidate "
     " not viable: no known conversion from %2 to %3 for "
     "%select{%ordinal5 argument|object argument}4; "
     "%select{|dereference the argument with *|"
-    "take the address of the argument with &}6">;
+    "take the address of the argument with &|"
+    "remove *|"
+    "remove &}6">;
 def note_ovl_candidate_bad_arc_conv : Note<"candidate "
     "%select{function|function|constructor|"
     "function |function |constructor |"
diff --git a/include/clang/Sema/Overload.h b/include/clang/Sema/Overload.h
index b3948c3de0..4f3d3e8f6f 100644
--- a/include/clang/Sema/Overload.h
+++ b/include/clang/Sema/Overload.h
@@ -531,7 +531,9 @@ namespace clang {
   enum OverloadFixItKind {
     OFIK_Undefined = 0,
     OFIK_Dereference,
-    OFIK_TakeAddress
+    OFIK_TakeAddress,
+    OFIK_RemoveDereference,
+    OFIK_RemoveTakeAddress
   };
 
   /// OverloadCandidate - A single candidate in an overload set (C++ 13.3).
@@ -565,7 +567,7 @@ namespace clang {
     /// The FixIt hints which can be used to fix the Bad candidate.
     struct FixInfo {
       /// The list of Hints (all have to be applied).
-      SmallVector<FixItHint, 4> Hints;
+      SmallVector<FixItHint, 1> Hints;
 
       /// The number of Conversions fixed. This can be different from the size
       /// of the Hints vector since we allow multiple FixIts per conversion.
diff --git a/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp
index 2f8bdc768f..969998eaa0 100644
--- a/lib/Sema/SemaOverload.cpp
+++ b/lib/Sema/SemaOverload.cpp
@@ -6752,8 +6752,15 @@ static bool TryToFixBadConversion(Sema &S,
   const SourceLocation End = S.PP.getLocForEndOfToken(Arg->getSourceRange()
                                                       .getEnd());
   bool NeedParen = true;
-  if (isa<ParenExpr>(Arg) || isa<DeclRefExpr>(Arg))
+  if (isa<ParenExpr>(Arg) ||
+      isa<DeclRefExpr>(Arg) ||
+      isa<ArraySubscriptExpr>(Arg) ||
+      isa<CallExpr>(Arg) ||
+      isa<MemberExpr>(Arg))
     NeedParen = false;
+  if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(Arg))
+    if (UO->isPostfix())
+      NeedParen = false;
 
   // Check if the argument needs to be dereferenced
   // (type * -> type) or (type * -> type &).
@@ -6765,13 +6772,20 @@ static bool TryToFixBadConversion(Sema &S,
     ImplicitConversionSequence ICS =
       TryCopyInitialization(S, &TmpExpr, ToQTy, true, true, false);
 
+    OverloadFixItKind FixKind = OFIK_Dereference;
     if (!ICS.isBad()) {
       // Do not suggest dereferencing a Null pointer.
       if (Arg->IgnoreParenCasts()->
           isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull))
         return false;
 
-      if (NeedParen) {
+      if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(Arg)) {
+        if (UO->getOpcode() == UO_AddrOf) {
+          ConvFix.Hints.push_back(
+                               FixItHint::CreateRemoval(Arg->getSourceRange()));
+          FixKind = OFIK_RemoveTakeAddress;
+        }
+      } else if (NeedParen) {
         ConvFix.Hints.push_back(FixItHint::CreateInsertion(Begin, "*("));
         ConvFix.Hints.push_back(FixItHint::CreateInsertion(End, ")"));
       } else {
@@ -6779,7 +6793,7 @@ static bool TryToFixBadConversion(Sema &S,
       }
       ConvFix.NumConversionsFixed++;
       if (ConvFix.NumConversionsFixed == 1)
-        ConvFix.Kind = OFIK_Dereference;
+        ConvFix.Kind = FixKind;
       return true;
     }
   }
@@ -6788,15 +6802,24 @@ static bool TryToFixBadConversion(Sema &S,
   // (type -> type *) or (type & -> type *).
   if (isa<PointerType>(ToQTy)) {
     // Only suggest taking address of L-values.
-    if (!Arg->isLValue())
+    if (!Arg->isLValue() || Arg->getObjectKind() != OK_Ordinary)
       return false;
 
     OpaqueValueExpr TmpExpr(Arg->getExprLoc(),
                             S.Context.getPointerType(FromQTy), VK_RValue);
     ImplicitConversionSequence ICS =
       TryCopyInitialization(S, &TmpExpr, ToQTy, true, true, false);
+
+    OverloadFixItKind FixKind = OFIK_TakeAddress;
     if (!ICS.isBad()) {
-      if (NeedParen) {
+
+      if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(Arg)) {
+        if (UO->getOpcode() == UO_Deref) {
+          ConvFix.Hints.push_back(
+                               FixItHint::CreateRemoval(Arg->getSourceRange()));
+          FixKind = OFIK_RemoveDereference;
+        }
+      } else if (NeedParen) {
         ConvFix.Hints.push_back(FixItHint::CreateInsertion(Begin, "&("));
         ConvFix.Hints.push_back(FixItHint::CreateInsertion(End, ")"));
       } else {
@@ -6804,7 +6827,7 @@ static bool TryToFixBadConversion(Sema &S,
       }
       ConvFix.NumConversionsFixed++;
       if (ConvFix.NumConversionsFixed == 1)
-        ConvFix.Kind = OFIK_TakeAddress;
+        ConvFix.Kind = FixKind;
       return true;
     }
   }
@@ -7004,7 +7027,7 @@ void DiagnoseBadConversion(Sema &S, OverloadCandidate *Cand, unsigned I) {
     << (unsigned) (Cand->Fix.Kind);
 
   // If we can fix the conversion, suggest the FixIts.
-  for (llvm::SmallVector<FixItHint, 4>::iterator
+  for (llvm::SmallVector<FixItHint, 1>::iterator
       HI = Cand->Fix.Hints.begin(), HE = Cand->Fix.Hints.end();
       HI != HE; ++HI)
     FDiag << *HI;
@@ -7365,11 +7388,12 @@ struct CompareOverloadCandidatesForDisplay {
         unsigned numRFixes = R->Fix.NumConversionsFixed;
         numLFixes = (numLFixes == 0) ? UINT_MAX : numLFixes;
         numRFixes = (numRFixes == 0) ? UINT_MAX : numRFixes;
-        if (numLFixes != numRFixes)
+        if (numLFixes != numRFixes) {
           if (numLFixes < numRFixes)
             return true;
           else
             return false;
+        }
 
         // If there's any ordering between the defined conversions...
         // FIXME: this might not be transitive.
diff --git a/test/FixIt/fixit-function-call.cpp b/test/FixIt/fixit-function-call.cpp
index e59e21b933..d954267f0e 100644
--- a/test/FixIt/fixit-function-call.cpp
+++ b/test/FixIt/fixit-function-call.cpp
@@ -24,7 +24,7 @@ void f2(intTy2 *a) {
 
 // This call cannot be fixed since without resulting in null pointer dereference.
 // CHECK: error: no matching function for call to 'f1
-// CHECK-NOT: take the address of the argument with &
+// CHECK-NOT: dereference the argument with *
 // CHECK-NOT: fix-it
   f1((int *)0);
 }
@@ -65,16 +65,19 @@ struct B : public A {
   double y;
 };
 
+class C : A {};
+
 bool br(A &a);
 bool bp(A *a);
 bool dv(B b);
 
-void dbcaller(A *ptra, B *ptrb) {
+void dbcaller(A *ptra, B *ptrb, C &c) {
   B b;
 
 // CHECK: error: no matching function for call to 'br
 // CHECK: fix-it{{.*}}*
   br(ptrb); // good
+
 // CHECK: error: no matching function for call to 'bp
 // CHECK: fix-it{{.*}}&
   bp(b); // good
@@ -82,6 +85,20 @@ void dbcaller(A *ptra, B *ptrb) {
 // CHECK: error: no matching function for call to 'dv
 // CHECK-NOT: fix-it
   dv(ptra); // bad: base to derived
+
+// CHECK: error: no matching function for call to 'dv
+// CHECK: remove &
+  dv(&b);
+
+// CHECK: error: no matching function for call to 'bp
+// CHECK: remove *
+  bp(*ptra);
+
+// TODO: Test that we do not provide a fixit when inheritance is private.
+// CHECK: error: no matching function for call to 'bp
+// There should not be a fixit here:
+// CHECK: fix-it
+  bp(c);
 }
 
 // CHECK: errors generated