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.

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

[clang-tidy] Improved cppcoreguidelines-narrowing-conversions.IgnoreConversionFromTypes #69242

Conversation

PiotrZSL
Copy link
Member

Extended IgnoreConversionFromTypes option to include
types without a declaration, such as built-in types.

…onversionFromTypes

Extended IgnoreConversionFromTypes option to include
types without a declaration, such as built-in types.
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 16, 2023

@llvm/pr-subscribers-clang-tidy

Author: Piotr Zegar (PiotrZSL)

Changes

Extended IgnoreConversionFromTypes option to include
types without a declaration, such as built-in types.


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

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp (+28-10)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp (+8-1)
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
index 1b858db511f50a2..45fef9471d5211e 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -14,6 +14,7 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/APSInt.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 
@@ -23,6 +24,26 @@ using namespace clang::ast_matchers;
 
 namespace clang::tidy::cppcoreguidelines {
 
+namespace {
+
+AST_MATCHER_P(QualType, hasAnyType, std::vector<StringRef>, Names) {
+  if (Names.empty())
+    return false;
+
+  std::string Name = Node.getLocalUnqualifiedType().getAsString();
+  return llvm::any_of(Names, [&Name](StringRef Ref) { return Ref == Name; });
+}
+
+AST_MATCHER(FieldDecl, hasIntBitwidth) {
+  assert(Node.isBitField());
+  const ASTContext &Ctx = Node.getASTContext();
+  unsigned IntBitWidth = Ctx.getIntWidth(Ctx.IntTy);
+  unsigned CurrentBitWidth = Node.getBitWidthValue(Ctx);
+  return IntBitWidth == CurrentBitWidth;
+}
+
+} // namespace
+
 NarrowingConversionsCheck::NarrowingConversionsCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
@@ -53,25 +74,22 @@ void NarrowingConversionsCheck::storeOptions(
   Options.store(Opts, "PedanticMode", PedanticMode);
 }
 
-AST_MATCHER(FieldDecl, hasIntBitwidth) {
-  assert(Node.isBitField());
-  const ASTContext &Ctx = Node.getASTContext();
-  unsigned IntBitWidth = Ctx.getIntWidth(Ctx.IntTy);
-  unsigned CurrentBitWidth = Node.getBitWidthValue(Ctx);
-  return IntBitWidth == CurrentBitWidth;
-}
-
 void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
   // ceil() and floor() are guaranteed to return integers, even though the type
   // is not integral.
   const auto IsCeilFloorCallExpr = expr(callExpr(callee(functionDecl(
       hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor")))));
 
+  std::vector<StringRef> IgnoreConversionFromTypesVec =
+      utils::options::parseStringList(IgnoreConversionFromTypes);
+
   // We may want to exclude other types from the checks, such as `size_type`
   // and `difference_type`. These are often used to count elements, represented
   // in 64 bits and assigned to `int`. Rarely are people counting >2B elements.
-  const auto IsConversionFromIgnoredType = hasType(namedDecl(
-      hasAnyName(utils::options::parseStringList(IgnoreConversionFromTypes))));
+  const auto IsConversionFromIgnoredType =
+      anyOf(hasType(namedDecl(hasAnyName(IgnoreConversionFromTypesVec))),
+            allOf(unless(hasType(namedDecl())),
+                  hasType(qualType(hasAnyType(IgnoreConversionFromTypesVec)))));
 
   // `IsConversionFromIgnoredType` will ignore narrowing calls from those types,
   // but not expressions that are promoted to an ignored type as a result of a
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index af164d0462d52c1..5ad12aadd5b55b3 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -217,6 +217,11 @@ Changes in existing checks
   coroutine functions and increase issue detection for cases involving type
   aliases with references.
 
+- Improved :doc:`cppcoreguidelines-narrowing-conversions
+  <clang-tidy/checks/cppcoreguidelines/narrowing-conversions>` check by
+  extending the `IgnoreConversionFromTypes` option to include types without a
+  declaration, such as built-in types.
+
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
   ignore delegate constructors.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
index ab9aabf44ff68c5..fbd38e4dc98d8bb 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/narrowing-conversions-ignoreconversionfromtypes-option.cpp
@@ -4,7 +4,7 @@
 // RUN: %check_clang_tidy -check-suffix=IGNORED %s \
 // RUN: cppcoreguidelines-narrowing-conversions %t -- \
 // RUN: -config='{CheckOptions: { \
-// RUN:   cppcoreguidelines-narrowing-conversions.IgnoreConversionFromTypes: "global_size_t;nested_size_type" \
+// RUN:   cppcoreguidelines-narrowing-conversions.IgnoreConversionFromTypes: "global_size_t;nested_size_type;long" \
 // RUN: }}'
 
 // We use global_size_t instead of 'size_t' because windows predefines size_t.
@@ -72,3 +72,10 @@ void most_narrowing_is_not_ok() {
   // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
   // CHECK-MESSAGES-IGNORED: :[[@LINE-2]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
 }
+
+void test_ignore_builtin_type_pr58809() {
+  long x = 123;
+  int y = x;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:11: warning: narrowing conversion from 'long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES-NOT-IGNORED: :[[@LINE-2]]:11: warning: narrowing conversion from 'long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}

@PiotrZSL PiotrZSL merged commit fd06155 into llvm:main Oct 26, 2023
5 checks passed
@PiotrZSL PiotrZSL deleted the 58809-clang-tidy-ignoreconversionfromtypes-has-no-effect-when-set-to-long branch October 26, 2023 05:59
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Oct 26, 2023
…onversionFromTypes (llvm#69242)

Extended IgnoreConversionFromTypes option to include
types without a declaration, such as built-in types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-tidy: IgnoreConversionFromTypes has no effect when set to 'long'
3 participants