Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert -Wformat for scoped enums #8638

Merged
merged 2 commits into from
May 1, 2024
Merged

Revert -Wformat for scoped enums #8638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37d7fe7
Revert "[Sema] Use underlying type of scoped enum for -Wformat diagno…
porglezomp Apr 24, 2024
2f6f637
Revert "Diagnose incorrect use of scoped enumerations in format strings"
porglezomp Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 4 additions & 7 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,10 @@ Improvements to Clang's diagnostics
- ``-Wformat`` cast fix-its will now suggest ``static_cast`` instead of C-style casts
for C++ code.
- ``-Wformat`` will no longer suggest a no-op fix-it for fixing scoped enum format
warnings. Instead, it will suggest casting the enum object based on its
underlying type.
warnings. Instead, it will suggest casting the enum object to the type specified
in the format string.
- Clang contexpr evaluator now displays notes as well as an error when a constructor
of a base class is not called in the constructor of its derived class.

- ``-Wzero-as-null-pointer-constant`` diagnostic is no longer emitted when using ``__null``
(or, more commonly, ``NULL`` when the platform defines it as ``__null``) to be more consistent
Expand Down Expand Up @@ -670,11 +672,6 @@ Bug Fixes in This Version
(`#50244 <https://github.com/llvm/llvm-project/issues/50244>`_).
- Apply ``-fmacro-prefix-map`` to anonymous tags in template arguments
(`#63219 <https://github.com/llvm/llvm-project/issues/63219>`_).
- Clang now properly diagnoses format string mismatches involving scoped
enumeration types. A scoped enumeration type is not promoted to an integer
type by the default argument promotions, and thus this is UB. Clang's
behavior now matches GCC's behavior in C++.
(`#38717 <https://github.com/llvm/llvm-project/issues/38717>`_).
- Fixed a failing assertion when implicitly defining a function within a GNU
statement expression that appears outside of a function block scope. The
assertion was benign outside of asserts builds and would only fire in C.
Expand Down
11 changes: 4 additions & 7 deletions clang/lib/AST/FormatString.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,12 +351,10 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
case AnyCharTy: {
if (const auto *ETy = argTy->getAs<EnumType>()) {
// If the enum is incomplete we know nothing about the underlying type.
// Assume that it's 'int'. Do not use the underlying type for a scoped
// enumeration.
// Assume that it's 'int'.
if (!ETy->getDecl()->isComplete())
return NoMatch;
if (ETy->isUnscopedEnumerationType())
argTy = ETy->getDecl()->getIntegerType();
argTy = ETy->getDecl()->getIntegerType();
}

if (const auto *BT = argTy->getAs<BuiltinType>()) {
Expand Down Expand Up @@ -393,11 +391,10 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
case SpecificTy: {
if (const EnumType *ETy = argTy->getAs<EnumType>()) {
// If the enum is incomplete we know nothing about the underlying type.
// Assume that it's 'int'. Do not use the underlying type for a scoped
// enumeration as that needs an exact match.
// Assume that it's 'int'.
if (!ETy->getDecl()->isComplete())
argTy = C.IntTy;
else if (ETy->isUnscopedEnumerationType())
else
argTy = ETy->getDecl()->getIntegerType();
}
argTy = C.getCanonicalType(argTy).getUnqualifiedType();
Expand Down
35 changes: 15 additions & 20 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11500,26 +11500,18 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
ImplicitMatch == ArgType::NoMatchTypeConfusion)
Match = ImplicitMatch;
assert(Match != ArgType::MatchPromotion);

// Look through unscoped enums to their underlying type.
// Look through enums to their underlying type.
bool IsEnum = false;
bool IsScopedEnum = false;
QualType IntendedTy = ExprTy;
if (auto EnumTy = ExprTy->getAs<EnumType>()) {
IntendedTy = EnumTy->getDecl()->getIntegerType();
if (EnumTy->isUnscopedEnumerationType()) {
ExprTy = IntendedTy;
// This controls whether we're talking about the underlying type or not,
// which we only want to do when it's an unscoped enum.
IsEnum = true;
} else {
IsScopedEnum = true;
}
ExprTy = EnumTy->getDecl()->getIntegerType();
IsEnum = true;
}

// %C in an Objective-C context prints a unichar, not a wchar_t.
// If the argument is an integer of some kind, believe the %C and suggest
// a cast instead of changing the conversion specifier.
QualType IntendedTy = ExprTy;
if (isObjCContext() &&
FS.getConversionSpecifier().getKind() == ConversionSpecifier::CArg) {
if (ExprTy->isIntegralOrUnscopedEnumerationType() &&
Expand Down Expand Up @@ -11555,10 +11547,8 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
std::tie(CastTy, CastTyName) = shouldNotPrintDirectly(S.Context, IntendedTy, E);
if (!CastTy.isNull()) {
// %zi/%zu and %td/%tu are OK to use for NSInteger/NSUInteger of type int
// (long in ASTContext). Only complain to pedants or when they're the
// underlying type of a scoped enum (which always needs a cast).
if (!IsScopedEnum &&
(CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
// (long in ASTContext). Only complain to pedants.
if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
(AT.isSizeT() || AT.isPtrdiffT()) &&
AT.matchesType(S.Context, CastTy))
Match = ArgType::NoMatchPedantic;
Expand Down Expand Up @@ -11613,15 +11603,20 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
// should be printed as 'long' for 64-bit compatibility.)
// Rather than emitting a normal format/argument mismatch, we want to
// add a cast to the recommended type (and correct the format string
// if necessary). We should also do so for scoped enumerations.
// if necessary).
SmallString<16> CastBuf;
llvm::raw_svector_ostream CastFix(CastBuf);
CastFix << (S.LangOpts.CPlusPlus ? "static_cast<" : "(");
IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
if (IsScopedEnum) {
CastFix << AT.getRepresentativeType(S.Context).getAsString(
S.Context.getPrintingPolicy());
} else {
IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
}
CastFix << (S.LangOpts.CPlusPlus ? ">" : ")");

SmallVector<FixItHint,4> Hints;
if (AT.matchesType(S.Context, IntendedTy) != ArgType::Match ||
if ((!AT.matchesType(S.Context, IntendedTy) && !IsScopedEnum) ||
ShouldNotPrintDirectly)
Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str()));

Expand Down Expand Up @@ -11649,7 +11644,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
Hints.push_back(FixItHint::CreateInsertion(After, ")"));
}

if (ShouldNotPrintDirectly && !IsScopedEnum) {
if (ShouldNotPrintDirectly) {
// The expression has a type that should not be printed directly.
// We extract the name from the typedef because we don't want to show
// the underlying type in the diagnostic.
Expand Down
35 changes: 0 additions & 35 deletions clang/test/FixIt/format-darwin-enum-class.cpp
View file
Open in desktop

This file was deleted.

20 changes: 2 additions & 18 deletions clang/test/FixIt/format.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,33 +12,21 @@ struct S {
N::E Type;
};

using uint32_t = unsigned;
enum class FixedE : uint32_t { Two };

void a(N::E NEVal, S *SPtr, S &SRef) {
printf("%d", N::E::One); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these expected-warning necessary on the Apple CI after the changes?

They seem to be failing in every configuration when we pick up this change in our CI.

I noticed that there was no "please test llvm" in this change, so the LLVM/Clang tests were not executed in this PR.

// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:")"

printf("%hd", N::E::One); // expected-warning{{format specifies type 'short' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:14}:"%d"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:")"
// CHECK: "static_cast<short>("

printf("%hu", N::E::One); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:14}:"%d"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:")"
// CHECK: "static_cast<unsigned short>("

LOG("%d", N::E::One); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:22-[[@LINE-2]]:22}:")"

LOG("%s", N::E::One); // expected-warning{{format specifies type 'char *' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:10}:"%d"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:")"

printf("%d", NEVal); // expected-warning{{format specifies type 'int' but the argument has type 'N::E'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:21-[[@LINE-2]]:21}:")"
Expand Down Expand Up @@ -70,8 +58,4 @@ void a(N::E NEVal, S *SPtr, S &SRef) {
SRef.Type);
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"static_cast<int>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:")"

printf("%u", FixedE::Two); //expected-warning{{format specifies type 'unsigned int' but the argument has type 'FixedE'}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast<uint32_t>("
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:27-[[@LINE-2]]:27}:")"
}
24 changes: 0 additions & 24 deletions clang/test/SemaCXX/format-strings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,28 +213,4 @@ void f() {


}

namespace ScopedEnumerations {
enum class Scoped1 { One };
enum class Scoped2 : unsigned short { Two };

void f(Scoped1 S1, Scoped2 S2) {
printf("%hhd", S1); // expected-warning {{format specifies type 'char' but the argument has type 'Scoped1'}}
printf("%hd", S1); // expected-warning {{format specifies type 'short' but the argument has type 'Scoped1'}}
printf("%d", S1); // expected-warning {{format specifies type 'int' but the argument has type 'Scoped1'}}

printf("%hhd", S2); // expected-warning {{format specifies type 'char' but the argument has type 'Scoped2'}}
printf("%hd", S2); // expected-warning {{format specifies type 'short' but the argument has type 'Scoped2'}}
printf("%d", S2); // expected-warning {{format specifies type 'int' but the argument has type 'Scoped2'}}

scanf("%hhd", &S1); // expected-warning {{format specifies type 'char *' but the argument has type 'Scoped1 *'}}
scanf("%hd", &S1); // expected-warning {{format specifies type 'short *' but the argument has type 'Scoped1 *'}}
scanf("%d", &S1); // expected-warning {{format specifies type 'int *' but the argument has type 'Scoped1 *'}}

scanf("%hhd", &S2); // expected-warning {{format specifies type 'char *' but the argument has type 'Scoped2 *'}}
scanf("%hd", &S2); // expected-warning {{format specifies type 'short *' but the argument has type 'Scoped2 *'}}
scanf("%d", &S2); // expected-warning {{format specifies type 'int *' but the argument has type 'Scoped2 *'}}
}
}

#endif