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

Tue May 16 13:06:28 PDT 2023

aaron.ballman added a comment.

In D148276#4324314 <https://reviews.llvm.org/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&patternType=regexp&sm=1&groupBy=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