This is an archive of the discontinued LLVM Phabricator instance.

[clang][Sema] Suggest static_cast in C++ code
ClosedPublic

Authored by abrachet on Jun 23 2023, 5:09 AM.

Details

Summary

This patch changes the -Wformat diagnostic to suggest static_cast over
a C-style cast for {,Objective}C++ when recommending the argument be
casted rather than changing the format string.

Before:

clang/test/FixIt/format.mm:11:16: warning: format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t' [-Wformat]
   11 |   NSLog(@"%C", wchar_data);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t'}}
      |           ~~   ^~~~~~~~~~
      |                (unsigned short)

After:

clang/test/FixIt/format.mm:11:16: warning: format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t' [-Wformat]
   11 |   NSLog(@"%C", wchar_data);  // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'wchar_t'}}
      |           ~~   ^~~~~~~~~~
      |                static_cast<unsigned short>( )

Diff Detail

Event Timeline

abrachet created this revision.Jun 23 2023, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 5:09 AM
abrachet requested review of this revision.Jun 23 2023, 5:09 AM

I'm generally fine with suggesting a static cast, but the only test case you're updating is a objective C one, isn't it? Isn't the fixit wrong there?

I'm generally fine with suggesting a static cast, but the only test case you're updating is a objective C one, isn't it? Isn't the fixit wrong there?

That test is Objective C++, I don't know much about the language, but I've compiled this TU with the fixit applied and it compiles fine

I couldn't think of a case to test this fixit in C++, save for D153623 where that case is tested

cjdb added a comment.Jun 27 2023, 9:28 AM

LGTM in general, but two things before approval:

  1. please add a description of when this is applicable, along with an example (including the output suggested by Clang) to your git commit message
  2. please update the release notes to have the same info

I'm not opposed to the changes but I also don't understand what's driving them either. Can you explain what problem you're solving, as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

Also, the changes need a release note and the patch needs more of a summary.

clang/lib/Sema/SemaChecking.cpp
10825–10827

It's a bit odd to me that we'd recommend a C++-style cast for an inherently C interface (format specifier strings), especially because that can be more of a choice of style than anything.

abrachet updated this revision to Diff 535074.Jun 27 2023, 11:26 AM
abrachet edited the summary of this revision. (Show Details)

I'm not opposed to the changes but I also don't understand what's driving them either. Can you explain what problem you're solving, as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

The main motivation here can be found in D153623, I've just split these patches into two to make them easier to review. The warning change in 3632e2f5179 caused a lot of new warnings for us so I made a made a fixit instead of needing to manually fixing them all.

Regarding clang being opinionated on the casting style, this is fairly common, I found [[ this | https://github.com/llvm/llvm-project/blob/d5ce68afdf65fd8906d9570c0a71d3557de70838/clang/lib/Sema/SemaExpr.cpp#L14622 ]] location that emits a static_cast suggestion in C++ mode and a C-style cast otherwise. There are also a couple of other fixits that unconditionally suggest static_cast but these are specific to C++ code. So I do think that fixits currently suggest static_cast over C-style casts. This changed was motivated by the fixit in D153623 which is only meaningful for C++

aaron.ballman accepted this revision.Jul 2 2023, 10:05 AM

I'm not opposed to the changes but I also don't understand what's driving them either. Can you explain what problem you're solving, as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

The main motivation here can be found in D153623, I've just split these patches into two to make them easier to review. The warning change in 3632e2f5179 caused a lot of new warnings for us so I made a made a fixit instead of needing to manually fixing them all.

Regarding clang being opinionated on the casting style, this is fairly common, I found [[ this | https://github.com/llvm/llvm-project/blob/d5ce68afdf65fd8906d9570c0a71d3557de70838/clang/lib/Sema/SemaExpr.cpp#L14622 ]] location that emits a static_cast suggestion in C++ mode and a C-style cast otherwise. There are also a couple of other fixits that unconditionally suggest static_cast but these are specific to C++ code. So I do think that fixits currently suggest static_cast over C-style casts. This changed was motivated by the fixit in D153623 which is only meaningful for C++

I brought this up because we support scoped enumerations in C as an extension, but C23 added scoped enumeration types officially (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3030.htm) and we've not yet determined how close or far from conformance we already are. However, a C-style cast in C and a C++-style cast in C++ is defensible. The part that is unfortunate is that I wouldn't apply that fix-it myself because I think a C++-style cast is too distracting for this particular use case. FWIW, I feel exactly the same way about the use of static_cast<void> for the -Wcomma case (added in https://reviews.llvm.org/D3976 without any mention of the choice being stylistic).

I won't block this patch, but I do think we should be very careful about making style choices like this with fix-its, as no matter which way we choose, some class of folks won't wish to apply the fix-it solely due to style, which lessens the utility of having the fix-it.

LGTM!

This revision is now accepted and ready to land.Jul 2 2023, 10:05 AM
cjdb added a comment.Jul 5 2023, 10:25 AM

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

If we wanted to remove this patch I would be fine with that. There already exists clang-tidy modernize that can turn C-style casts to their C++ counterparts. So I think that is a fine compromise. Though a hesitation is that there is not git-clang-tidy (to my knowledge) that will only touch changed lines.

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

If this is in reference to moving D153623 out of clang and into clang-tidy, I wouldn't object to abandoning these two patches. Though to be frank, it's unlikely that I would contribute this to clang-tidy.

cjdb added a comment.Jul 10 2023, 9:44 AM

If we wanted to remove this patch I would be fine with that. There already exists clang-tidy modernize that can turn C-style casts to their C++ counterparts. So I think that is a fine compromise. Though a hesitation is that there is not git-clang-tidy (to my knowledge) that will only touch changed lines.

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

If this is in reference to moving D153623 out of clang and into clang-tidy, I wouldn't object to abandoning these two patches. Though to be frank, it's unlikely that I would contribute this to clang-tidy.

It's a reference to what Aaron has said in this patch:

as this seems like a question of style (we traditionally don't concern fix-its with making style choices)?

We typically put style stuff into clang-tidy.

With what @aaron.ballman has said in mind, should we be adding this to Clang, or should it be a clang-tidy thing?

I (weakly) think we should keep it in Clang: I think we should have fix-its when possible and this is a case that can really save folks some head-scratching and avoid UB, but if we're going to have a fix-it, we need to pick *some* cast style to use, which means we're making a kind of style choice no matter what we do.

I think users who have strong preferences that a particular cast style is used consistently could then use clang-tidy to clean up the style part later. That's not entirely different from how we handle things like whitespace decisions with fix-its and clang-format's role in cleaning up those changes. We don't have such a check currently, but it seems plausible to add such a check should there be sufficient demand for it.

cjdb accepted this revision.Jul 10 2023, 11:05 AM
This revision was landed with ongoing or failed builds.Jul 14 2023, 9:25 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 9:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript