[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D148276#4324314 , @sousajo wrote:

> have been sick, and could not advance much except I added the tests to 
> replicate the issue. Any ideas on how to proceed here?

Sorry to hear you've been sick, but thank you for your patience while I thought 
about this further. Two points worth observing:

1. GCC diagnoses the problematic code the same way your patch does: 
https://godbolt.org/z/44GnM9jzE
2. Clang and GCC both diagnose the notionally equivalent code using a C-style 
cast: https://godbolt.org/z/bbTPYb5rE

Based on that, it seems defensible for us to diagnose that code by re-landing 
the patch. However, I wonder why the changes broke anything. We build with GCC 
all the time. Is `-Wcast-qual` only enabled for bots building with Clang?

However, I also feel like this behavior is somewhat user-hostile because it 
seems reasonable to expect the developer is well aware that they're casting 
away the `const` qualifier when using something named `remove_const_t` to 
specify the cast destination type, so it seems potentially reasonable to 
silence the diagnostic in those cases (consistently, for both function- and 
c-style casts). It seems to be a reasonably popular approach to casting away 
const in the wild: 
https://sourcegraph.com/search?q=context:global+%5Cb%5Bstd::%5D%3Fremove_const%5B_t%5D%3F%5C%3C.*%5C%3E%5B::type%5D%3F%5C%28.*%5C%29=regexp=1=group
 so this might allow more folks to opt into `-Wcast-qual`. But implementing 
that would be tricky because we'd have to look through the cast expression to 
see whether it came from a type trait, and we typically do not want the 
frontend to be inspecting AST nodes by name (like looking for particular 
identifiers and changing behavior based on that), and we'd be introducing 
another divergence between the Clang and GCC behaviors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Jorge Pinto Sousa via Phabricator via cfe-commits]

sousajo added a comment.

have been sick, and could not advance much except I added the tests to 
replicate the issue. Any ideas on how to proceed here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Thurston Dang via Phabricator via cfe-commits]

thurston added a comment.

Thanks Aaron and Jorge for the quick revert and replies! :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Jorge Pinto Sousa via Phabricator via cfe-commits]

sousajo added a comment.

In D148276#4288236 , @aaron.ballman 
wrote:

> In D148276#4288145 , @sousajo wrote:
>
>> hey :( I will try to investigate it a bit sometime next week ^^ thanks for 
>> pointing it out
>
> Thank you for looking into it! I didn't think about `remove_const`, 
> `remove_cv`, etc. when considering test cases, sorry about that!

No worries :) Ill add those and try to craft a fix. If I am really lost I also 
let you know ^^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D148276#4288145 , @sousajo wrote:

> hey :( I will try to investigate it a bit sometime next week ^^ thanks for 
> pointing it out

Thank you for looking into it! I didn't think about `remove_const`, 
`remove_cv`, etc. when considering test cases, sorry about that!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Jorge Pinto Sousa via Phabricator via cfe-commits]

sousajo added a comment.

hey :( I will try to investigate it a bit sometime next week ^^ thanks for 
pointing it out


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman reopened this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Reverted in 7f00ecd029213aee9baf297554b0cb25a5ffbd0f 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D148276#4287931 , @aaron.ballman 
wrote:

> In D148276#4287900 , @thurston 
> wrote:
>
>> I think this change is triggering an error on a buildbot 
>> (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):
>>
>>   
>> /b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45:
>>  error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops 
>> const qualifier [-Werror,-Wcast-qual]
>> std::remove_const_t(Ctx),
>>
>> even though the code is deliberately trying to remove the const qualifier 
>> the right (?) way.
>
> Ouch, that's pretty sad. I'll work around it and land a fix shortly, thank 
> you for pointing this out!

Actually, that's going to be more involved than a quick fix. I think I'll 
revert the patch and reopen this review so that @sousajo can investigate the 
proper fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D148276#4287900 , @thurston wrote:

> I think this change is triggering an error on a buildbot 
> (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):
>
>   
> /b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45:
>  error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops 
> const qualifier [-Werror,-Wcast-qual]
> std::remove_const_t(Ctx),
>
> even though the code is deliberately trying to remove the const qualifier the 
> right (?) way.

Ouch, that's pretty sad. I'll work around it and land a fix shortly, thank you 
for pointing this out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Thurston Dang via Phabricator via cfe-commits]

thurston added a comment.

I think this change is triggering an error on a buildbot 
(https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):

  
/b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45:
 error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops 
const qualifier [-Werror,-Wcast-qual]
std::remove_const_t(Ctx),

even though the code is deliberately trying to remove the const qualifier the 
right (?) way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

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

[PATCH] D148276: [clang] trigger -Wcast-qual on functional casts

[Aaron Ballman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7c0021923503: [clang] trigger -Wcast-qual on functional 
casts (authored by sousajo, committed by aaron.ballman).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D148276?vs=513583=515680#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148276

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaCast.cpp
  clang/test/SemaCXX/warn-cast-qual.cpp

Index: clang/test/SemaCXX/warn-cast-qual.cpp
===
--- clang/test/SemaCXX/warn-cast-qual.cpp
+++ clang/test/SemaCXX/warn-cast-qual.cpp
@@ -34,6 +34,17 @@
   const int  = (int &)((int &)a);   // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   const int  = (int &)((const int &)a); // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   const int  = (const int &)((int &)a); // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+
+  using T = int&;
+  using T2 = const int&;
+  const int  =T2(a);  // no warning
+  int a22 = T(a); // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  const int  = T(a);  // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  int  = T(T2(a));// expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  int  = T(T(a)); // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  const int  = T(T(a));   // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  int  = T(T2(a));// expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  const int  = T2(T(a));  // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
 }
 
 void foo_1() {
@@ -49,6 +60,17 @@
   volatile int  = (int &)((int &)a);  // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   volatile int  = (int &)((volatile int &)a); // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   volatile int  = (volatile int &)((int &)a); // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+
+  using T = int&;
+  using T2 = volatile int&;
+  volatile int  =T2(a); // no warning
+  int a22 = T(a);   // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  volatile int  = T(a); // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  int  = T(T2(a));  // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  int  = T(T(a));   // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  volatile int  = T(T(a));  // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  int  = T(T2(a));  // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  volatile int  = T2(T(a)); // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
 }
 
 void foo_2() {
@@ -64,6 +86,17 @@
   const volatile int  = (int &)((int &)a);// expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   const volatile int  = (int &)((const volatile int &)a); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   const volatile int  = (const volatile int &)((int &)a); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+
+  using T = int&;
+  using T2 = const volatile int&;
+  const volatile int  =T2(a);   // no warning
+  int a22 = T(a); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int  = T(a); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  int  = T(T2(a));// expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  int  = T(T(a)); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int  = T(T(a));  // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int  = T(T2(a)); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int  = T2(T(a)); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
 }
 
 void bar_0() {
@@ -78,6 +111,16 @@
 
   const