[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG46b8ea2fff90: [clang-tidy] Add check for implicit widening 
of multiplication result (authored by lebedev.ri).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-extint.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-pointer-offset.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-short.cpp
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10097,7 +10097,12 @@
 return getVectorType(getCorrespondingUnsignedType(VTy->getElementType()),
  VTy->getNumElements(), VTy->getVectorKind());
 
-  // For enums, we return the unsigned version of the base type.
+  // For _ExtInt, return an unsigned _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/true, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
   if (const auto *ETy = T->getAs())
 T = ETy->getDecl()->getIntegerType();
 
@@ -10150,6 +10155,74 @@
   }
 }
 
+QualType ASTContext::getCorrespondingSignedType(QualType T) const {
+  assert((T->hasUnsignedIntegerRepresentation() ||
+  T->isUnsignedFixedPointType()) &&
+ "Unexpected type");
+
+  // Turn <4 x unsigned int> -> <4 x signed int>
+  if (const auto *VTy = T->getAs())
+return getVectorType(getCorrespondingSignedType(VTy->getElementType()),
+ VTy->getNumElements(), VTy->getVectorKind());
+
+  // For _ExtInt, return a signed _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/false, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+
+  switch (T->castAs()->getKind()) {
+  case BuiltinType::Char_U:
+  case BuiltinType::UChar:
+return SignedCharTy;
+  case BuiltinType::UShort:
+return ShortTy;
+  case BuiltinType::UInt:
+return IntTy;
+  case BuiltinType::ULong:
+return LongTy;
+  case BuiltinType::ULongLong:
+return LongLongTy;
+  case BuiltinType::UInt128:
+return Int128Ty;
+  // wchar_t is special. It is either unsigned or not, but when it's unsigned,
+  // there's no matching "signed wchar_t". Therefore we return the signed
+  // version of it's underlying type instead.
+  case BuiltinType::WChar_U:
+return getSignedWCharType();
+
+  case BuiltinType::UShortAccum:
+return ShortAccumTy;
+  case BuiltinType::UAccum:
+return AccumTy;
+  case BuiltinType::ULongAccum:
+return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+return ShortFractTy;
+  case BuiltinType::UFract:
+return FractTy;
+  case BuiltinType::ULongFract:
+return LongFractTy;
+  case BuiltinType::SatUShortFract:
+return SatShortFractTy;
+  case BuiltinType::SatUFract:
+return SatFractTy;
+  case BuiltinType::SatULongFract:
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");
+  }
+}
+
 ASTMutationListener::~ASTMutationListener() = default;
 
 void ASTMutationListener::DeducedReturnType(const FunctionDecl *FD,
Index: clang/include/clang/AST/ASTContext.h
===
--- 

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@aaron.ballman thank you so much for the review!




Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+  IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+

aaron.ballman wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > One thing that's awkward about this is that there's no portable 
> > > > `ssize_t` type -- that's a POSIX type but it doesn't exist on all 
> > > > platforms (like Windows). We shouldn't print out a typecast that's 
> > > > going to cause compile errors, but we also shouldn't use the underlying 
> > > > type for `ssize_t` as that may be incorrect for other target 
> > > > architectures.
> > > I'm still not quite certain what to do about this. Would it make sense to 
> > > use the underlying type on platforms that don't have `ssize_t`? 
> > > Relatedly, if we're going to suggest this as a replacement, we should 
> > > also insert an include for the correct header file.
> > I've been thinking about this, and i really can't come up with a better fix 
> > than using `ptrdiff_t`.
> I'm not super excited about using `ptrdiff_t` as there are not likely to be 
> pointer subtractions in the vicinity of the code being diagnosed, but at the 
> same time, `ptrdiff_t` is functionally the same as `size_t` except with a 
> sign bit, which is what `ssize_t` is. I'll hold my nose for now, but if we 
> get bug reports about this use, we may want to investigate more involved 
> solutions.
Yep. `signed size_t` isn't a valid type, so at best i guess we could avoid 
emitting such a fixit.
In C++, `std::make_signed::type` is valid, but it's kinda mouthful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 337214.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

Address final nits.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-extint.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-pointer-offset.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-short.cpp
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10097,7 +10097,12 @@
 return getVectorType(getCorrespondingUnsignedType(VTy->getElementType()),
  VTy->getNumElements(), VTy->getVectorKind());
 
-  // For enums, we return the unsigned version of the base type.
+  // For _ExtInt, return an unsigned _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/true, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
   if (const auto *ETy = T->getAs())
 T = ETy->getDecl()->getIntegerType();
 
@@ -10150,6 +10155,74 @@
   }
 }
 
+QualType ASTContext::getCorrespondingSignedType(QualType T) const {
+  assert((T->hasUnsignedIntegerRepresentation() ||
+  T->isUnsignedFixedPointType()) &&
+ "Unexpected type");
+
+  // Turn <4 x unsigned int> -> <4 x signed int>
+  if (const auto *VTy = T->getAs())
+return getVectorType(getCorrespondingSignedType(VTy->getElementType()),
+ VTy->getNumElements(), VTy->getVectorKind());
+
+  // For _ExtInt, return a signed _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/false, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+
+  switch (T->castAs()->getKind()) {
+  case BuiltinType::Char_U:
+  case BuiltinType::UChar:
+return SignedCharTy;
+  case BuiltinType::UShort:
+return ShortTy;
+  case BuiltinType::UInt:
+return IntTy;
+  case BuiltinType::ULong:
+return LongTy;
+  case BuiltinType::ULongLong:
+return LongLongTy;
+  case BuiltinType::UInt128:
+return Int128Ty;
+  // wchar_t is special. It is either unsigned or not, but when it's unsigned,
+  // there's no matching "signed wchar_t". Therefore we return the signed
+  // version of it's underlying type instead.
+  case BuiltinType::WChar_U:
+return getSignedWCharType();
+
+  case BuiltinType::UShortAccum:
+return ShortAccumTy;
+  case BuiltinType::UAccum:
+return AccumTy;
+  case BuiltinType::ULongAccum:
+return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+return ShortFractTy;
+  case BuiltinType::UFract:
+return FractTy;
+  case BuiltinType::ULongFract:
+return LongFractTy;
+  case BuiltinType::SatUShortFract:
+return SatShortFractTy;
+  case BuiltinType::SatUFract:
+return SatFractTy;
+  case BuiltinType::SatULongFract:
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");
+  }
+}
+
 ASTMutationListener::~ASTMutationListener() = default;
 
 void ASTMutationListener::DeducedReturnType(const FunctionDecl *FD,
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2748,6 +2748,14 @@
   // a given fixed point type.
   

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some small nits in the documentation.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+  IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+

lebedev.ri wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > One thing that's awkward about this is that there's no portable `ssize_t` 
> > > type -- that's a POSIX type but it doesn't exist on all platforms (like 
> > > Windows). We shouldn't print out a typecast that's going to cause compile 
> > > errors, but we also shouldn't use the underlying type for `ssize_t` as 
> > > that may be incorrect for other target architectures.
> > I'm still not quite certain what to do about this. Would it make sense to 
> > use the underlying type on platforms that don't have `ssize_t`? Relatedly, 
> > if we're going to suggest this as a replacement, we should also insert an 
> > include for the correct header file.
> I've been thinking about this, and i really can't come up with a better fix 
> than using `ptrdiff_t`.
I'm not super excited about using `ptrdiff_t` as there are not likely to be 
pointer subtractions in the vicinity of the code being diagnosed, but at the 
same time, `ptrdiff_t` is functionally the same as `size_t` except with a sign 
bit, which is what `ssize_t` is. I'll hold my nose for now, but if we get bug 
reports about this use, we may want to investigate more involved solutions.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst:40
+   When suggesting fix-its for C++ code, should C++-style ``static_cast<>()``'s
+   be suggested, or C-style casts.
+

Add info about the default behavior.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst:45
+   When suggesting to include the appropriate header in C++ code,
+   should  header be suggested, or .
+

Same here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 336985.
lebedev.ri added a comment.

Hardcode triple/target in tests, should fix 32-bit CI.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-extint.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-pointer-offset.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-short.cpp
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10097,7 +10097,12 @@
 return getVectorType(getCorrespondingUnsignedType(VTy->getElementType()),
  VTy->getNumElements(), VTy->getVectorKind());
 
-  // For enums, we return the unsigned version of the base type.
+  // For _ExtInt, return an unsigned _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/true, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
   if (const auto *ETy = T->getAs())
 T = ETy->getDecl()->getIntegerType();
 
@@ -10150,6 +10155,74 @@
   }
 }
 
+QualType ASTContext::getCorrespondingSignedType(QualType T) const {
+  assert((T->hasUnsignedIntegerRepresentation() ||
+  T->isUnsignedFixedPointType()) &&
+ "Unexpected type");
+
+  // Turn <4 x unsigned int> -> <4 x signed int>
+  if (const auto *VTy = T->getAs())
+return getVectorType(getCorrespondingSignedType(VTy->getElementType()),
+ VTy->getNumElements(), VTy->getVectorKind());
+
+  // For _ExtInt, return a signed _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/false, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+
+  switch (T->castAs()->getKind()) {
+  case BuiltinType::Char_U:
+  case BuiltinType::UChar:
+return SignedCharTy;
+  case BuiltinType::UShort:
+return ShortTy;
+  case BuiltinType::UInt:
+return IntTy;
+  case BuiltinType::ULong:
+return LongTy;
+  case BuiltinType::ULongLong:
+return LongLongTy;
+  case BuiltinType::UInt128:
+return Int128Ty;
+  // wchar_t is special. It is either unsigned or not, but when it's unsigned,
+  // there's no matching "signed wchar_t". Therefore we return the signed
+  // version of it's underlying type instead.
+  case BuiltinType::WChar_U:
+return getSignedWCharType();
+
+  case BuiltinType::UShortAccum:
+return ShortAccumTy;
+  case BuiltinType::UAccum:
+return AccumTy;
+  case BuiltinType::ULongAccum:
+return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+return ShortFractTy;
+  case BuiltinType::UFract:
+return FractTy;
+  case BuiltinType::ULongFract:
+return LongFractTy;
+  case BuiltinType::SatUShortFract:
+return SatShortFractTy;
+  case BuiltinType::SatUFract:
+return SatFractTy;
+  case BuiltinType::SatULongFract:
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");
+  }
+}
+
 ASTMutationListener::~ASTMutationListener() = default;
 
 void ASTMutationListener::DeducedReturnType(const FunctionDecl *FD,
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2748,6 +2748,14 @@
   // a given fixed point type.
   QualType 

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 336981.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.

@aaron.ballman thank you for taking a look!
Addressed all review notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-extint.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-pointer-offset.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-short.cpp
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10097,7 +10097,12 @@
 return getVectorType(getCorrespondingUnsignedType(VTy->getElementType()),
  VTy->getNumElements(), VTy->getVectorKind());
 
-  // For enums, we return the unsigned version of the base type.
+  // For _ExtInt, return an unsigned _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/true, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
   if (const auto *ETy = T->getAs())
 T = ETy->getDecl()->getIntegerType();
 
@@ -10150,6 +10155,74 @@
   }
 }
 
+QualType ASTContext::getCorrespondingSignedType(QualType T) const {
+  assert((T->hasUnsignedIntegerRepresentation() ||
+  T->isUnsignedFixedPointType()) &&
+ "Unexpected type");
+
+  // Turn <4 x unsigned int> -> <4 x signed int>
+  if (const auto *VTy = T->getAs())
+return getVectorType(getCorrespondingSignedType(VTy->getElementType()),
+ VTy->getNumElements(), VTy->getVectorKind());
+
+  // For _ExtInt, return a signed _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/false, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+
+  switch (T->castAs()->getKind()) {
+  case BuiltinType::Char_U:
+  case BuiltinType::UChar:
+return SignedCharTy;
+  case BuiltinType::UShort:
+return ShortTy;
+  case BuiltinType::UInt:
+return IntTy;
+  case BuiltinType::ULong:
+return LongTy;
+  case BuiltinType::ULongLong:
+return LongLongTy;
+  case BuiltinType::UInt128:
+return Int128Ty;
+  // wchar_t is special. It is either unsigned or not, but when it's unsigned,
+  // there's no matching "signed wchar_t". Therefore we return the signed
+  // version of it's underlying type instead.
+  case BuiltinType::WChar_U:
+return getSignedWCharType();
+
+  case BuiltinType::UShortAccum:
+return ShortAccumTy;
+  case BuiltinType::UAccum:
+return AccumTy;
+  case BuiltinType::ULongAccum:
+return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+return ShortFractTy;
+  case BuiltinType::UFract:
+return FractTy;
+  case BuiltinType::ULongFract:
+return LongFractTy;
+  case BuiltinType::SatUShortFract:
+return SatShortFractTy;
+  case BuiltinType::SatUFract:
+return SatFractTy;
+  case BuiltinType::SatULongFract:
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");
+  }
+}
+
 ASTMutationListener::~ASTMutationListener() = default;
 
 void ASTMutationListener::DeducedReturnType(const FunctionDecl *FD,
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ 

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+  IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+

aaron.ballman wrote:
> aaron.ballman wrote:
> > One thing that's awkward about this is that there's no portable `ssize_t` 
> > type -- that's a POSIX type but it doesn't exist on all platforms (like 
> > Windows). We shouldn't print out a typecast that's going to cause compile 
> > errors, but we also shouldn't use the underlying type for `ssize_t` as that 
> > may be incorrect for other target architectures.
> I'm still not quite certain what to do about this. Would it make sense to use 
> the underlying type on platforms that don't have `ssize_t`? Relatedly, if 
> we're going to suggest this as a replacement, we should also insert an 
> include for the correct header file.
I've been thinking about this, and i really can't come up with a better fix 
than using `ptrdiff_t`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

FWIW, it looks like tests are still failing on Windows according to the CI 
pipeline.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:126
+  QualType SizeTy = IndexExprType->isSignedIntegerType() ? SSizeTy : USizeTy;
+  // FIXME: is there a way to actually get the QualType for size_t/ssize_t?
+  StringRef TyAsString =

lebedev.ri wrote:
> aaron.ballman wrote:
> > You already are using the way?
> No, that's not it, because `SizeTy.getAsString()` will not be 
> `size_t`/`ssize_t`, but `long`/etc.
Ah, I see what you mean now, thanks.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+  IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+

aaron.ballman wrote:
> One thing that's awkward about this is that there's no portable `ssize_t` 
> type -- that's a POSIX type but it doesn't exist on all platforms (like 
> Windows). We shouldn't print out a typecast that's going to cause compile 
> errors, but we also shouldn't use the underlying type for `ssize_t` as that 
> may be incorrect for other target architectures.
I'm still not quite certain what to do about this. Would it make sense to use 
the underlying type on platforms that don't have `ssize_t`? Relatedly, if we're 
going to suggest this as a replacement, we should also insert an include for 
the correct header file.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp:10
+
+long t0(char a, char b) {
+  return a * b;

lebedev.ri wrote:
> aaron.ballman wrote:
> > Can you also test the fixit behavior in addition to the diagnostic behavior 
> > (at least one test for each kind of fix-it)?
> I would love to do that, but as we have briefly talked with @njames93,
> fix-it testing in clang-tidy, as compared to clang proper,
> is rudimentary. I basically can not add tests here.
> As far as i can see, it doesn't want to apply fix-its, because there are two 
> of them.
> 
> So this is the best i can do, take it or leave it. /s
Ah, thank you for clarifying the issue that the two fixits make it hard to test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:29
+  // Is this:  long r = int(x) * int(y);  ?
+  // FIXME: shall we skip brackets/casts/etc?
+  auto *BO = dyn_cast(E);

aaron.ballman wrote:
> I think we should skip parens at the very least. `x * y` should check the 
> same as `(x) * (y)`.
This is more about the future case for which there's FIXME few lines down.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:75
+  else if (Ty->isSignedIntegerType()) {
+assert(ETy->isUnsignedIntegerType() &&
+   "Expected source type to be signed.");

lebedev.ri wrote:
> aaron.ballman wrote:
> > Might be worth it to have tests around `signed char`, `unsigned char`, and 
> > `char` explicitly, as that gets awkward.
> Are you asking for test coverage, or for explicit handling of those types?
> I'll add tests.
It seems to kinda just work, because of the promotion to `int`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:126
+  QualType SizeTy = IndexExprType->isSignedIntegerType() ? SSizeTy : USizeTy;
+  // FIXME: is there a way to actually get the QualType for size_t/ssize_t?
+  StringRef TyAsString =

aaron.ballman wrote:
> You already are using the way?
No, that's not it, because `SizeTy.getAsString()` will not be 
`size_t`/`ssize_t`, but `long`/etc.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp:10
+
+long t0(char a, char b) {
+  return a * b;

aaron.ballman wrote:
> Can you also test the fixit behavior in addition to the diagnostic behavior 
> (at least one test for each kind of fix-it)?
I would love to do that, but as we have briefly talked with @njames93,
fix-it testing in clang-tidy, as compared to clang proper,
is rudimentary. I basically can not add tests here.
As far as i can see, it doesn't want to apply fix-its, because there are two of 
them.

So this is the best i can do, take it or leave it. /s



Comment at: clang/lib/AST/ASTContext.cpp:10158
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+

lebedev.ri wrote:
> aaron.ballman wrote:
> > This looks to be getting the same value as in the 
> > `getCorrespondingUnsignedType()` call?
> Yep. Are both of them incorrect?
> I'm not sure what should be returned here.
Actually, this is correct.
Note that we don't return it, but the later code will flip it's sign.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 335169.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.

@aaron.ballman thank you for taking a look!
Addressing all review notes other than `ssize_t` and `static_cast<>()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-extint.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-pointer-offset.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-short.cpp
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10097,7 +10097,12 @@
 return getVectorType(getCorrespondingUnsignedType(VTy->getElementType()),
  VTy->getNumElements(), VTy->getVectorKind());
 
-  // For enums, we return the unsigned version of the base type.
+  // For _ExtInt, return an unsigned _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/true, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
   if (const auto *ETy = T->getAs())
 T = ETy->getDecl()->getIntegerType();
 
@@ -10150,6 +10155,74 @@
   }
 }
 
+QualType ASTContext::getCorrespondingSignedType(QualType T) const {
+  assert((T->hasUnsignedIntegerRepresentation() ||
+  T->isUnsignedFixedPointType()) &&
+ "Unexpected type");
+
+  // Turn <4 x unsigned int> -> <4 x signed int>
+  if (const auto *VTy = T->getAs())
+return getVectorType(getCorrespondingSignedType(VTy->getElementType()),
+ VTy->getNumElements(), VTy->getVectorKind());
+
+  // For _ExtInt, return a signed _ExtInt with same width.
+  if (const auto *EITy = T->getAs())
+return getExtIntType(/*IsUnsigned=*/false, EITy->getNumBits());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+
+  switch (T->castAs()->getKind()) {
+  case BuiltinType::Char_U:
+  case BuiltinType::UChar:
+return SignedCharTy;
+  case BuiltinType::UShort:
+return ShortTy;
+  case BuiltinType::UInt:
+return IntTy;
+  case BuiltinType::ULong:
+return LongTy;
+  case BuiltinType::ULongLong:
+return LongLongTy;
+  case BuiltinType::UInt128:
+return Int128Ty;
+  // wchar_t is special. It is either unsigned or not, but when it's unsigned,
+  // there's no matching "signed wchar_t". Therefore we return the signed
+  // version of it's underlying type instead.
+  case BuiltinType::WChar_U:
+return getSignedWCharType();
+
+  case BuiltinType::UShortAccum:
+return ShortAccumTy;
+  case BuiltinType::UAccum:
+return AccumTy;
+  case BuiltinType::ULongAccum:
+return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+return ShortFractTy;
+  case BuiltinType::UFract:
+return FractTy;
+  case BuiltinType::ULongFract:
+return LongFractTy;
+  case BuiltinType::SatUShortFract:
+return SatShortFractTy;
+  case BuiltinType::SatUFract:
+return SatFractTy;
+  case BuiltinType::SatULongFract:
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");
+  }
+}
+
 ASTMutationListener::~ASTMutationListener() = default;
 
 void ASTMutationListener::DeducedReturnType(const FunctionDecl *FD,
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ 

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:67
+  << FixItHint::CreateInsertion(E->getBeginLoc(),
+"(" + Ty.getAsString() + ")(")
+  << FixItHint::CreateInsertion(E->getEndLoc(), ")") << 
E->getSourceRange();

We might want to consider letting the user select between `static_cast` and 
C-style casts via an option.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:130
+  StringRef TyAsString =
+  IndexExprType->isSignedIntegerType() ? "ssize_t" : "size_t";
+

One thing that's awkward about this is that there's no portable `ssize_t` type 
-- that's a POSIX type but it doesn't exist on all platforms (like Windows). We 
shouldn't print out a typecast that's going to cause compile errors, but we 
also shouldn't use the underlying type for `ssize_t` as that may be incorrect 
for other target architectures.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:75
+  else if (Ty->isSignedIntegerType()) {
+assert(ETy->isUnsignedIntegerType() &&
+   "Expected source type to be signed.");

lebedev.ri wrote:
> aaron.ballman wrote:
> > Might be worth it to have tests around `signed char`, `unsigned char`, and 
> > `char` explicitly, as that gets awkward.
> Are you asking for test coverage, or for explicit handling of those types?
> I'll add tests.
Just test coverage to make sure we're doing the right thing for them and not 
triggering these assertions.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst:6-9
+The check diagnoses instances where a result of a multiplication is implicitly
+widened, and suggests (with fix-it) to either silence the code by making
+widening explicit, or to perform the multiplication in a wider type,
+to avoid the widening afterwards.

I think the documentation would be stronger if it explained why this diagnostic 
is helpful (show some examples of bugs that it catches and that the suggested 
fixes remove the bug).



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp:10
+
+long t0(char a, char b) {
+  return a * b;

Can you also test the fixit behavior in addition to the diagnostic behavior (at 
least one test for each kind of fix-it)?



Comment at: clang/lib/AST/ASTContext.cpp:10158
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+

lebedev.ri wrote:
> aaron.ballman wrote:
> > This looks to be getting the same value as in the 
> > `getCorrespondingUnsignedType()` call?
> Yep. Are both of them incorrect?
> I'm not sure what should be returned here.
I think I confused myself on the code, I think both are correct.



Comment at: clang/lib/AST/ASTContext.cpp:10204
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");

lebedev.ri wrote:
> aaron.ballman wrote:
> > It looks like `_ExtInt` is missing from both this function and the 
> > corresponding unsigned one.
> Likewise, i'm not sure what should be returned for that.
> Just the original `_ExtInt`?
`_ExtInt` has `signed` and `unsigned` variants. You can use 
`ASTContext::getExtIntType()` to get the type with the correct signedness. 
However, I see now that it's not a builtin type (huh, I thought it was!), so 
it'd have to be above the `switch` but after the `enum` handling.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 334929.
lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.

Addressed review comments.
Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-char.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-int.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-pointer-offset.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-short.cpp
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10097,7 +10097,8 @@
 return getVectorType(getCorrespondingUnsignedType(VTy->getElementType()),
  VTy->getNumElements(), VTy->getVectorKind());
 
-  // For enums, we return the unsigned version of the base type.
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
   if (const auto *ETy = T->getAs())
 T = ETy->getDecl()->getIntegerType();
 
@@ -10150,6 +10151,70 @@
   }
 }
 
+QualType ASTContext::getCorrespondingSignedType(QualType T) const {
+  assert((T->hasUnsignedIntegerRepresentation() ||
+  T->isUnsignedFixedPointType()) &&
+ "Unexpected type");
+
+  // Turn <4 x unsigned int> -> <4 x signed int>
+  if (const auto *VTy = T->getAs())
+return getVectorType(getCorrespondingSignedType(VTy->getElementType()),
+ VTy->getNumElements(), VTy->getVectorKind());
+
+  // For enums, get the underlying integer type of the enum, and let the general
+  // integer type signchanging code handle it.
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+
+  switch (T->castAs()->getKind()) {
+  case BuiltinType::Char_U:
+  case BuiltinType::UChar:
+return SignedCharTy;
+  case BuiltinType::UShort:
+return ShortTy;
+  case BuiltinType::UInt:
+return IntTy;
+  case BuiltinType::ULong:
+return LongTy;
+  case BuiltinType::ULongLong:
+return LongLongTy;
+  case BuiltinType::UInt128:
+return Int128Ty;
+  // wchar_t is special. It is either unsigned or not, but when it's unsigned,
+  // there's no matching "signed wchar_t". Therefore we return the signed
+  // version of it's underlying type instead.
+  case BuiltinType::WChar_U:
+return getSignedWCharType();
+
+  case BuiltinType::UShortAccum:
+return ShortAccumTy;
+  case BuiltinType::UAccum:
+return AccumTy;
+  case BuiltinType::ULongAccum:
+return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+return ShortFractTy;
+  case BuiltinType::UFract:
+return FractTy;
+  case BuiltinType::ULongFract:
+return LongFractTy;
+  case BuiltinType::SatUShortFract:
+return SatShortFractTy;
+  case BuiltinType::SatUFract:
+return SatFractTy;
+  case BuiltinType::SatULongFract:
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");
+  }
+}
+
 ASTMutationListener::~ASTMutationListener() = default;
 
 void ASTMutationListener::DeducedReturnType(const FunctionDecl *FD,
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2748,6 +2748,14 @@
   // a given fixed point type.
   QualType getCorrespondingUnsignedType(QualType T) const;
 
+  // Per C99 6.2.5p6, for every signed integer type, there is a corresponding
+  // unsigned integer type.  This method takes an unsigned type, and returns the
+  // corresponding signed integer type.
+  // With the introduction of fixed point types in ISO N1169, this method also
+  // accepts fixed point types and returns the corresponding signed type for
+  // a given 

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

In D93822#2664157 , @aaron.ballman 
wrote:

> The CI is showing build failures and there are some clang-tidy nits to be 
> addressed as well.

Thank you for taking a look!




Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:32
+  if (!BO || BO->getOpcode() != BO_Mul)
+// FIXME: what about:  long r = int(x) + (int(y) * int(z));  ?
+return nullptr;

aaron.ballman wrote:
> I think that could be handled in a follow-up if we find the need, WDYT?
Sure.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:75
+  else if (Ty->isSignedIntegerType()) {
+assert(ETy->isUnsignedIntegerType() &&
+   "Expected source type to be signed.");

aaron.ballman wrote:
> Might be worth it to have tests around `signed char`, `unsigned char`, and 
> `char` explicitly, as that gets awkward.
Are you asking for test coverage, or for explicit handling of those types?
I'll add tests.



Comment at: clang/lib/AST/ASTContext.cpp:10158
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+

aaron.ballman wrote:
> This looks to be getting the same value as in the 
> `getCorrespondingUnsignedType()` call?
Yep. Are both of them incorrect?
I'm not sure what should be returned here.



Comment at: clang/lib/AST/ASTContext.cpp:10204
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");

aaron.ballman wrote:
> It looks like `_ExtInt` is missing from both this function and the 
> corresponding unsigned one.
Likewise, i'm not sure what should be returned for that.
Just the original `_ExtInt`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

The CI is showing build failures and there are some clang-tidy nits to be 
addressed as well.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:29
+  // Is this:  long r = int(x) * int(y);  ?
+  // FIXME: shall we skip brackets/casts/etc?
+  auto *BO = dyn_cast(E);

I think we should skip parens at the very least. `x * y` should check the same 
as `(x) * (y)`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:32
+  if (!BO || BO->getOpcode() != BO_Mul)
+// FIXME: what about:  long r = int(x) + (int(y) * int(z));  ?
+return nullptr;

I think that could be handled in a follow-up if we find the need, WDYT?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:75
+  else if (Ty->isSignedIntegerType()) {
+assert(ETy->isUnsignedIntegerType() &&
+   "Expected source type to be signed.");

Might be worth it to have tests around `signed char`, `unsigned char`, and 
`char` explicitly, as that gets awkward.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp:126
+  QualType SizeTy = IndexExprType->isSignedIntegerType() ? SSizeTy : USizeTy;
+  // FIXME: is there a way to actually get the QualType for size_t/ssize_t?
+  StringRef TyAsString =

You already are using the way?



Comment at: clang/lib/AST/ASTContext.cpp:10158
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+

This looks to be getting the same value as in the 
`getCorrespondingUnsignedType()` call?



Comment at: clang/lib/AST/ASTContext.cpp:10204
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");

It looks like `_ExtInt` is missing from both this function and the 
corresponding unsigned one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

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

[PATCH] D93822: [clang-tidy] Add check for implicit widening of multiplication result

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 57.
lebedev.ri retitled this revision from "[clang][Sema] Add diagnostics for 
implicit widening of multiplication result" to "[clang-tidy] Add check for 
implicit widening of multiplication result".
lebedev.ri edited the summary of this revision.
lebedev.ri edited reviewers, added: njames93; removed: rsmith, rjmccall, 
erichkeane, dblaikie.
lebedev.ri added a project: clang-tools-extra.
lebedev.ri added a comment.
Herald added subscribers: xazax.hun, mgorny.

Hmm. I kinda dropped the ball here.
As discussed, move it into a clang-tidy check.
Looks good now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-implicit-widening-of-multiplication-result.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-array-subscript-expression.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result-pointer-offset.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-implicit-widening-of-multiplication-result.cpp
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10143,6 +10143,69 @@
   }
 }
 
+QualType ASTContext::getCorrespondingSignedType(QualType T) const {
+  assert((T->hasUnsignedIntegerRepresentation() ||
+  T->isUnsignedFixedPointType()) &&
+ "Unexpected type");
+
+  // Turn <4 x unsigned int> -> <4 x signed int>
+  if (const auto *VTy = T->getAs())
+return getVectorType(getCorrespondingSignedType(VTy->getElementType()),
+ VTy->getNumElements(), VTy->getVectorKind());
+
+  // For enums, we return the signed version of the base type.
+  if (const auto *ETy = T->getAs())
+T = ETy->getDecl()->getIntegerType();
+
+  switch (T->castAs()->getKind()) {
+  case BuiltinType::Char_U:
+  case BuiltinType::UChar:
+return SignedCharTy;
+  case BuiltinType::UShort:
+return ShortTy;
+  case BuiltinType::UInt:
+return IntTy;
+  case BuiltinType::ULong:
+return LongTy;
+  case BuiltinType::ULongLong:
+return LongLongTy;
+  case BuiltinType::UInt128:
+return Int128Ty;
+  // wchar_t is special. It is either unsigned or not, but when it's unsigned,
+  // there's no matching "signed wchar_t". Therefore we return the signed
+  // version of it's underlying type instead.
+  case BuiltinType::WChar_U:
+return getSignedWCharType();
+
+  case BuiltinType::UShortAccum:
+return ShortAccumTy;
+  case BuiltinType::UAccum:
+return AccumTy;
+  case BuiltinType::ULongAccum:
+return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+return ShortFractTy;
+  case BuiltinType::UFract:
+return FractTy;
+  case BuiltinType::ULongFract:
+return LongFractTy;
+  case BuiltinType::SatUShortFract:
+return SatShortFractTy;
+  case BuiltinType::SatUFract:
+return SatFractTy;
+  case BuiltinType::SatULongFract:
+return SatLongFractTy;
+  default:
+llvm_unreachable("Unexpected unsigned integer or fixed point type");
+  }
+}
+
 ASTMutationListener::~ASTMutationListener() = default;
 
 void ASTMutationListener::DeducedReturnType(const FunctionDecl *FD,
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2748,6 +2748,14 @@
   // a given fixed point type.
   QualType getCorrespondingUnsignedType(QualType T) const;
 
+  // Per C99 6.2.5p6, for every signed integer type, there is a corresponding
+  // unsigned integer type.  This method takes an unsigned type, and returns the
+  // corresponding signed integer type.
+  // With the introduction of fixed point types in ISO N1169, this method also
+  // accepts fixed point types and returns the corresponding signed type for
+  // a given fixed point type.
+  QualType getCorrespondingSignedType(QualType T) const;
+
   // Per ISO N1169, this method accepts fixed point types and returns the
   // corresponding saturated type for a given fixed point type.
   QualType getCorrespondingSaturatedType(QualType Ty) const;
Index: