[clang] Remove diagnostic that came with [[clang::preferred_type]]

[Endilll]

#69104 introduce a diagnostic that checked underlying type of an enum against type of bit-field that is annotated with [[clang::preferred_type]]. When I tried to introduce this annotation in #70349, it turned out to be too chatty, despite effort to avoid that.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)

Changes

#69104 introduce a diagnostic that checked underlying type of an enum against type of bit-field that is annotated with [[clang::preferred_type]]. When I tried to introduce this annotation in #70349, it turned out to be too chatty, despite effort to avoid that.

---

Full diff: https://github.com/llvm/llvm-project/pull/70632.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticGroups.td (-1)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (-3)
• (modified) clang/lib/Sema/SemaDeclAttr.cpp (-22)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 9a8f3f03b39d165..4c4820d17b7184c 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -54,7 +54,6 @@ def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion",
                                            [SingleBitBitFieldConstantConversion]>;
 def BitFieldEnumConversion : DiagGroup<"bitfield-enum-conversion">;
 def BitFieldWidth : DiagGroup<"bitfield-width">;
-def BitFieldType : DiagGroup<"bitfield-type">;
 def CompoundTokenSplitByMacro : DiagGroup<"compound-token-split-by-macro">;
 def CompoundTokenSplitBySpace : DiagGroup<"compound-token-split-by-space">;
 def CompoundTokenSplit : DiagGroup<"compound-token-split",
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 453bd8a9a340425..70831fa0d91f2e3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3167,9 +3167,6 @@ def err_invalid_branch_protection_spec : Error<
   "invalid or misplaced branch protection specification '%0'">;
 def warn_unsupported_branch_protection_spec : Warning<
   "unsupported branch protection specification '%0'">, InGroup<BranchProtection>;
-def warn_attribute_underlying_type_mismatch : Warning<
-  "underlying type %0 of enumeration %1 doesn't match bit-field type %2">,
-  InGroup<BitFieldType>;
 
 def warn_unsupported_target_attribute
     : Warning<"%select{unsupported|duplicate|unknown}0%select{| CPU|"
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index fc4e3ccf29a6051..03ec8ece0e4e124 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5928,28 +5928,6 @@ static void handlePreferredTypeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   S.RequireCompleteType(ParmTSI->getTypeLoc().getBeginLoc(), QT,
                         diag::err_incomplete_type);
 
-  if (QT->isEnumeralType()) {
-    auto IsCorrespondingType = [&](QualType LHS, QualType RHS) {
-      assert(LHS != RHS);
-      if (LHS->isSignedIntegerType())
-        return LHS == S.getASTContext().getCorrespondingSignedType(RHS);
-      return LHS == S.getASTContext().getCorrespondingUnsignedType(RHS);
-    };
-    QualType BitfieldType =
-        cast<FieldDecl>(D)->getType()->getCanonicalTypeUnqualified();
-    QualType EnumUnderlyingType = QT->getAs<EnumType>()
-                                      ->getDecl()
-                                      ->getIntegerType()
-                                      ->getCanonicalTypeUnqualified();
-    if (EnumUnderlyingType != BitfieldType &&
-        !IsCorrespondingType(EnumUnderlyingType, BitfieldType)) {
-      S.Diag(ParmTSI->getTypeLoc().getBeginLoc(),
-             diag::warn_attribute_underlying_type_mismatch)
-          << EnumUnderlyingType << QT << BitfieldType;
-      return;
-    }
-  }
-
   D->addAttr(::new (S.Context) PreferredTypeAttr(S.Context, AL, ParmTSI));
 }
 

[cor3ntin]

I think we should keep the diagnostics, but only emit it when the type of the enum has a sizeof that is greater than the type of the bitfields.

Another solution would be to compare the number of bits of the greater enumerator with the number of bits in the bitfield.

[Endilll]

Another solution would be to compare the number of bits of the greater enumerator with the number of bits in the bitfield.

I've had this idea myself, but it's defeated by likes of

llvm-project/clang/include/clang/AST/DeclarationName.h

Lines 174 to 185 in e79f050

	enum StoredNameKind {
	StoredIdentifier = 0,
	StoredObjCZeroArgSelector = Selector::ZeroArg,
	StoredObjCOneArgSelector = Selector::OneArg,
	StoredCXXConstructorName = 3,
	StoredCXXDestructorName = 4,
	StoredCXXConversionFunctionName = 5,
	StoredCXXOperatorName = 6,
	StoredDeclarationNameExtra = Selector::MultiArg,
	PtrMask = 7,
	UncommonNameKindOffset = 8
	};

where UncommonNameKindOffset is an enumerator that is not there to be stored in memory.

[Endilll]

I think we should keep the diagnostics, but only emit it when the type of the enum has a sizeof that is greater than the type of the bitfields.

We can do that, but it's going to miss so much that I'm not sure it's worth our time at all.

[erichkeane]

I don't really see value for keeping this one, this was not my preferred use case in the original patch.

I Still like the 'range' one I suggested, and I don't find the UncommonNameKindOffset to be particularly motivating.

[erichkeane]

Why doesn't this result in any changes to tests? I would expect having to remove them, right?

[Endilll]

Why doesn't this result in any changes to tests? I would expect having to remove them, right?

There was a single test that was relying on sign mismatch, and then during code review we made the diagnostic to ignore signness. So it ended up merged without test case where this diagnostic was triggered.

[erichkeane]

Why doesn't this result in any changes to tests? I would expect having to remove them, right?

There was a single test that was relying on sign mismatch, and then during code review we made the diagnostic to ignore signness. So it ended up merged without test case where this diagnostic was triggered.

Ooof, that is disappointing. We should have never let this in without test coverage.

[AaronBallman]

Because we're on the fence about whether this has value and we're getting early feedback about diagnostic chattiness and we lack adequate test coverage for the changes, I think it's reasonable to back this out for now.

It might make more sense as a clang-tidy check should we want something along these lines.