Fix #58958 on github

[krsch]

Summary

Do not suggest to take the address of a const pointer to get void*

[dblaikie]

clang/lib/Sema/SemaFixItUtils.cpp

135–140

Do we need to check the constness here? What other cases does it come up? Like maybe if it's volatile we'd want to do the same thing?

[krsch]

clang/lib/Sema/SemaFixItUtils.cpp

135–140

For volatile pointers the same bad suggestion appears. Maybe I should remove the isConstQualified check as IIRC all non-const non-volatile pointers can be implicitly converted to void* so the suggestion would not appear for them. Probably any suggestion to change T* to T** for casting to void* is unlikely to be correct.

[xazax.hun]

clang/lib/Sema/SemaFixItUtils.cpp

136–137

Nit: we usually try to avoid repeating types.

136–137

Although the rest of the function clearly does not follow this guideline so feel free to leave it as is.

[dblaikie]

clang/lib/Sema/SemaFixItUtils.cpp

135–140

yeah, that's what I was thinking - probably remove the const check and maybe add a test case showing the volatile case too

[krsch]

Check any pointer, not just const. Test for volatile

[krsch]

clang/lib/Sema/SemaFixItUtils.cpp

136–137

The rest of the function was made this way in commit 1e95bc0f4063e, so I thought this is the style for this part of the codebase

[dblaikie]

Looks good, thanks!

Do you need me to commit this for you, or can you commit it yourself?

[aeubanks]

please make the title of the commit say what the commit is actually doing rather than only reference a bug number, the bug number should be in the description

[krsch]

Please commit it yourself as I don't have a commit access. This is my first patch here.

In D138426#3944472, @dblaikie wrote:

Do you need me to commit this for you, or can you commit it yourself?

[krsch]

Should I change the title myself or you can change it during commit? If it's on me, how do I change it? git commit --amend; arc diff?

[aeubanks]

In D138426#3946202, @krsch wrote:

Should I change the title myself or you can change it during commit? If it's on me, how do I change it? git commit --amend; arc diff?

phabricator isn't very elegant, locally changing the commit message doesn't propagate to phabricator even with arc diff, you have to change it on phabricator, then to propagate it locally arc amend

[dblaikie]

In D138426#3947044, @aeubanks wrote:

In D138426#3946202, @krsch wrote:

Should I change the title myself or you can change it during commit? If it's on me, how do I change it? git commit --amend; arc diff?

phabricator isn't very elegant, locally changing the commit message doesn't propagate to phabricator even with arc diff, you have to change it on phabricator, then to propagate it locally arc amend

That's OK, I'll just change it when I commit - I've got something lined up but didn't get it through yesterday.

clang/lib/Sema/SemaFixItUtils.cpp

if (CanConvert) {
       if (NumConversionsFixed == 1)
         Kind = FixKind;
       return true;
     }
   }
 
   // Check if the pointer to the argument needs to be passed:
   //   (type -> type *) or (type & -> type *).
-  if (isa<PointerType>(ToQTy)) {
+  if (const auto *ToPtrTy = dyn_cast<PointerType>(ToQTy)) {
     bool CanConvert = false;
     OverloadFixItKind FixKind = OFIK_TakeAddress;
 
     // Only suggest taking address of L-values.
     if (!Expr->isLValue() || Expr->getObjectKind() != OK_Ordinary)
       return false;
 
+    // Do no take address of const pointer to get void*
+    if (isa<PointerType>(FromQTy) && ToPtrTy->isVoidPointerType())
+      return false;
+
     CanConvert = CompareTypes(S.Context.getPointerType(FromQTy), ToQTy, S,
                               Begin, VK_PRValue);
     if (CanConvert) {
 
       if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(Expr)) {
         if (UO->getOpcode() == UO_Deref) {
           FixKind = OFIK_RemoveDereference;
           Hints.push_back(FixItHint::CreateRemoval(
                             CharSourceRange::getTokenRange(Begin, Begin)));
         }

clang/test/FixIt/fixit-function-call.cpp

 
 // CHECK: no matching function for call to 'u'
 // CHECK: candidate function not viable: no known conversion from 'C' to 'const C *' for 1st argument; take the address of the argument with &
 // CHECK: candidate function not viable
 // CHECK: candidate function not viable
   u(c);
 }
 
+void accept_void(void*);
+
+void issue58958(const char* a, volatile char * v, const volatile char * cv) {
+// CHECK: no matching function for call to 'accept_void'
+// CHECK-NOT: take the address of the argument with &
+    accept_void(a);
+// CHECK: no matching function for call to 'accept_void'
+// CHECK-NOT: take the address of the argument with &
+    accept_void(v);
+// CHECK: no matching function for call to 'accept_void'
+// CHECK-NOT: take the address of the argument with &
+    accept_void(cv);
+    char b;
+// CHECK: no matching function for call to 'accept_void'
+// CHECK: take the address of the argument with &
+    accept_void(b);
+// CHECK-NOT: no matching function for call to 'accept_void'
+// CHECK-NOT: take the address of the argument with &
+    accept_void(&b);
+}
+
 // CHECK: errors generated