[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-11-02 Thread Vlad Serebrennikov via cfe-commits 

https://github.com/Endilll closed 
https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-31 Thread Aaron Ballman via cfe-commits 

https://github.com/AaronBallman approved this pull request.

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.

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread Erich Keane via cfe-commits 


@@ -5928,28 +5928,6 @@ static void handlePreferredTypeAttr(Sema , Decl *D, 
const ParsedAttr ) {
   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(D)->getType()->getCanonicalTypeUnqualified();
-QualType EnumUnderlyingType = QT->getAs()
-  ->getDecl()
-  ->getIntegerType()
-  ->getCanonicalTypeUnqualified();
-if (EnumUnderlyingType != BitfieldType &&
-!IsCorrespondingType(EnumUnderlyingType, BitfieldType)) {

erichkeane wrote:

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.  

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread Erich Keane via cfe-commits 

erichkeane wrote:

> > 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.

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread Vlad Serebrennikov via cfe-commits 

Endilll wrote:

> 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.

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread Erich Keane via cfe-commits 

https://github.com/erichkeane commented:

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

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread Vlad Serebrennikov via cfe-commits 


@@ -5928,28 +5928,6 @@ static void handlePreferredTypeAttr(Sema , Decl *D, 
const ParsedAttr ) {
   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(D)->getType()->getCanonicalTypeUnqualified();
-QualType EnumUnderlyingType = QT->getAs()
-  ->getDecl()
-  ->getIntegerType()
-  ->getCanonicalTypeUnqualified();
-if (EnumUnderlyingType != BitfieldType &&
-!IsCorrespondingType(EnumUnderlyingType, BitfieldType)) {

Endilll wrote:

> 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.

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread Vlad Serebrennikov via cfe-commits 


@@ -5928,28 +5928,6 @@ static void handlePreferredTypeAttr(Sema , Decl *D, 
const ParsedAttr ) {
   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(D)->getType()->getCanonicalTypeUnqualified();
-QualType EnumUnderlyingType = QT->getAs()
-  ->getDecl()
-  ->getIntegerType()
-  ->getCanonicalTypeUnqualified();
-if (EnumUnderlyingType != BitfieldType &&
-!IsCorrespondingType(EnumUnderlyingType, BitfieldType)) {

Endilll wrote:

> 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 
https://github.com/llvm/llvm-project/blob/e79f0506cf6fd32ed466fdd2ccbb68dcacbb4bf5/clang/include/clang/AST/DeclarationName.h#L174-L185
where `UncommonNameKindOffset` is an enumerator that is not there to be stored 
in memory.

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread via cfe-commits 


@@ -5928,28 +5928,6 @@ static void handlePreferredTypeAttr(Sema , Decl *D, 
const ParsedAttr ) {
   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(D)->getType()->getCanonicalTypeUnqualified();
-QualType EnumUnderlyingType = QT->getAs()
-  ->getDecl()
-  ->getIntegerType()
-  ->getCanonicalTypeUnqualified();
-if (EnumUnderlyingType != BitfieldType &&
-!IsCorrespondingType(EnumUnderlyingType, BitfieldType)) {

cor3ntin wrote:

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.

https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)


Changes

https://github.com/llvm/llvm-project/pull/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 
https://github.com/llvm/llvm-project/pull/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
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;
-def warn_attribute_underlying_type_mismatch : Warning<
-  "underlying type %0 of enumeration %1 doesn't match bit-field type %2">,
-  InGroup;
 
 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 , Decl *D, 
const ParsedAttr ) {
   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(D)->getType()->getCanonicalTypeUnqualified();
-QualType EnumUnderlyingType = QT->getAs()
-  ->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));
 }
 

``




https://github.com/llvm/llvm-project/pull/70632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Remove diagnostic that came with `[[clang::preferred_type]]` (PR #70632)

2023-10-30 Thread Vlad Serebrennikov via cfe-commits 

https://github.com/Endilll created 
https://github.com/llvm/llvm-project/pull/70632

https://github.com/llvm/llvm-project/pull/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 
https://github.com/llvm/llvm-project/pull/70349, it turned out to be too 
chatty, despite effort to avoid that.

>From 63ca596224b3150b4225279bd3b07ce42a7fa86c Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Mon, 30 Oct 2023 10:44:28 +0300
Subject: [PATCH] [clang] Remove diagnostic that came with
 [[clang::preferred_type]]

---
 clang/include/clang/Basic/DiagnosticGroups.td |  1 -
 .../clang/Basic/DiagnosticSemaKinds.td|  3 ---
 clang/lib/Sema/SemaDeclAttr.cpp   | 22 ---
 3 files changed, 26 deletions(-)

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;
-def warn_attribute_underlying_type_mismatch : Warning<
-  "underlying type %0 of enumeration %1 doesn't match bit-field type %2">,
-  InGroup;
 
 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 , Decl *D, 
const ParsedAttr ) {
   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(D)->getType()->getCanonicalTypeUnqualified();
-QualType EnumUnderlyingType = QT->getAs()
-  ->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));
 }
 

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