[PATCH] D138426: Fix #58958 on github

[Alexey Kreshchuk via Phabricator via cfe-commits]

krsch created this revision.
krsch added reviewers: NoQ, xazax.hun.
Herald added a subscriber: rnkovacs.
Herald added a project: All.
krsch requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138426

Files:
  clang/lib/Sema/SemaFixItUtils.cpp
  clang/test/FixIt/fixit-function-call.cpp


Index: clang/test/FixIt/fixit-function-call.cpp
===
--- clang/test/FixIt/fixit-function-call.cpp
+++ clang/test/FixIt/fixit-function-call.cpp
@@ -115,4 +115,16 @@
   u(c);
 }
 
+void accept_void(void*);
+
+void issue58958(const char* a) {
+// CHECK: no matching function for call to 'accept_void'
+// CHECK-NOT: take the address of the argument with &
+accept_void(a);
+char b;
+// CHECK: no matching function for call to 'accept_void'
+// CHECK: take the address of the argument with &
+accept_void(b);
+}
+
 // CHECK: errors generated
Index: clang/lib/Sema/SemaFixItUtils.cpp
===
--- clang/lib/Sema/SemaFixItUtils.cpp
+++ clang/lib/Sema/SemaFixItUtils.cpp
@@ -132,6 +132,13 @@
 if (!Expr->isLValue() || Expr->getObjectKind() != OK_Ordinary)
   return false;
 
+// Do no take address of const pointer to get void*
+const PointerType *FromPtrTy = dyn_cast(FromQTy);
+const PointerType *ToPtrTy = dyn_cast(ToQTy);
+if (FromPtrTy && FromPtrTy->getPointeeType().isConstQualified() &&
+ToPtrTy->isVoidPointerType())
+  return false;
+
 CanConvert = CompareTypes(S.Context.getPointerType(FromQTy), ToQTy, S,
   Begin, VK_PRValue);
 if (CanConvert) {


Index: clang/test/FixIt/fixit-function-call.cpp
===
--- clang/test/FixIt/fixit-function-call.cpp
+++ clang/test/FixIt/fixit-function-call.cpp
@@ -115,4 +115,16 @@
   u(c);
 }
 
+void accept_void(void*);
+
+void issue58958(const char* a) {
+// CHECK: no matching function for call to 'accept_void'
+// CHECK-NOT: take the address of the argument with &
+accept_void(a);
+char b;
+// CHECK: no matching function for call to 'accept_void'
+// CHECK: take the address of the argument with &
+accept_void(b);
+}
+
 // CHECK: errors generated
Index: clang/lib/Sema/SemaFixItUtils.cpp
===
--- clang/lib/Sema/SemaFixItUtils.cpp
+++ clang/lib/Sema/SemaFixItUtils.cpp
@@ -132,6 +132,13 @@
 if (!Expr->isLValue() || Expr->getObjectKind() != OK_Ordinary)
   return false;
 
+// Do no take address of const pointer to get void*
+const PointerType *FromPtrTy = dyn_cast(FromQTy);
+const PointerType *ToPtrTy = dyn_cast(ToQTy);
+if (FromPtrTy && FromPtrTy->getPointeeType().isConstQualified() &&
+ToPtrTy->isVoidPointerType())
+  return false;
+
 CanConvert = CompareTypes(S.Context.getPointerType(FromQTy), ToQTy, S,
   Begin, VK_PRValue);
 if (CanConvert) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138426: Fix #58958 on github

[Alexey Kreshchuk via Phabricator via cfe-commits]

krsch added inline comments.



Comment at: clang/lib/Sema/SemaFixItUtils.cpp:135-140
+// Do no take address of const pointer to get void*
+const PointerType *FromPtrTy = dyn_cast(FromQTy);
+const PointerType *ToPtrTy = dyn_cast(ToQTy);
+if (FromPtrTy && FromPtrTy->getPointeeType().isConstQualified() &&
+ToPtrTy->isVoidPointerType())
+  return false;

dblaikie wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138426

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

[PATCH] D138426: Fix #58958 on github

[Alexey Kreshchuk via Phabricator via cfe-commits]

krsch updated this revision to Diff 477166.
krsch added a comment.

Check any pointer, not just const. Test for volatile


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138426

Files:
  clang/lib/Sema/SemaFixItUtils.cpp
  clang/test/FixIt/fixit-function-call.cpp


Index: clang/test/FixIt/fixit-function-call.cpp
===
--- clang/test/FixIt/fixit-function-call.cpp
+++ clang/test/FixIt/fixit-function-call.cpp
@@ -115,4 +115,25 @@
   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
Index: clang/lib/Sema/SemaFixItUtils.cpp
===
--- clang/lib/Sema/SemaFixItUtils.cpp
+++ clang/lib/Sema/SemaFixItUtils.cpp
@@ -124,7 +124,7 @@
 
   // Check if the pointer to the argument needs to be passed:
   //   (type -> type *) or (type & -> type *).
-  if (isa(ToQTy)) {
+  if (const auto *ToPtrTy = dyn_cast(ToQTy)) {
 bool CanConvert = false;
 OverloadFixItKind FixKind = OFIK_TakeAddress;
 
@@ -132,6 +132,10 @@
 if (!Expr->isLValue() || Expr->getObjectKind() != OK_Ordinary)
   return false;
 
+// Do no take address of const pointer to get void*
+if (isa(FromQTy) && ToPtrTy->isVoidPointerType())
+  return false;
+
 CanConvert = CompareTypes(S.Context.getPointerType(FromQTy), ToQTy, S,
   Begin, VK_PRValue);
 if (CanConvert) {


Index: clang/test/FixIt/fixit-function-call.cpp
===
--- clang/test/FixIt/fixit-function-call.cpp
+++ clang/test/FixIt/fixit-function-call.cpp
@@ -115,4 +115,25 @@
   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
Index: clang/lib/Sema/SemaFixItUtils.cpp
===
--- clang/lib/Sema/SemaFixItUtils.cpp
+++ clang/lib/Sema/SemaFixItUtils.cpp
@@ -124,7 +124,7 @@
 
   // Check if the pointer to the argument needs to be passed:
   //   (type -> type *) or (type & -> type *).
-  if (isa(ToQTy)) {
+  if (const auto *ToPtrTy = dyn_cast(ToQTy)) {
 bool CanConvert = false;
 OverloadFixItKind FixKind = OFIK_TakeAddress;
 
@@ -132,6 +132,10 @@
 if (!Expr->isLValue() || Expr->getObjectKind() != OK_Ordinary)
   return false;
 
+// Do no take address of const pointer to get void*
+if (isa(FromQTy) && ToPtrTy->isVoidPointerType())
+  return false;
+
 CanConvert = CompareTypes(S.Context.getPointerType(FromQTy), ToQTy, S,
   Begin, VK_PRValue);
 if (CanConvert) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138426: Fix #58958 on github

[Alexey Kreshchuk via Phabricator via cfe-commits]

krsch added inline comments.



Comment at: clang/lib/Sema/SemaFixItUtils.cpp:136-137
+// Do no take address of const pointer to get void*
+const PointerType *FromPtrTy = dyn_cast(FromQTy);
+const PointerType *ToPtrTy = dyn_cast(ToQTy);
+if (FromPtrTy && FromPtrTy->getPointeeType().isConstQualified() &&

xazax.hun wrote:
> xazax.hun wrote:
> > Nit: we usually try to avoid repeating types.
> Although the rest of the function clearly does not follow this guideline so 
> feel free to leave it as is.
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138426

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

[PATCH] D138426: Fix #58958 on github

[Alexey Kreshchuk via Phabricator via cfe-commits]

krsch added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138426

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

[PATCH] D138426: Fix #58958 on github

[Alexey Kreshchuk via Phabricator via cfe-commits]

krsch added a comment.

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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138426

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