Author: serge-sans-paille Date: 2026-04-14T20:59:09Z New Revision: 08932ddde177a7f9dacf5d34e209c48f3d52800e
URL: https://github.com/llvm/llvm-project/commit/08932ddde177a7f9dacf5d34e209c48f3d52800e DIFF: https://github.com/llvm/llvm-project/commit/08932ddde177a7f9dacf5d34e209c48f3d52800e.diff LOG: [clang-tidy] Detect std::rot[lr] pattern within modernize.use-std-bit (#186324) Basically turning `x << N | x >> (64 - N)` into `std::rotl(x, N)`. Added: Modified: clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.cpp clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.h clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-bit.rst clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-bit.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.cpp index e649c1c5aadda..0abe7d6323bdf 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.cpp @@ -8,6 +8,7 @@ #include "UseStdBitCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/Support/FormatVariadic.h" using namespace clang::ast_matchers; @@ -17,7 +18,8 @@ UseStdBitCheck::UseStdBitCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), IncludeInserter(Options.getLocalOrGlobal("IncludeStyle", utils::IncludeSorter::IS_LLVM), - areDiagsSelfContained()) {} + areDiagsSelfContained()), + HonorIntPromotion(Options.get("HonorIntPromotion", false)) {} void UseStdBitCheck::registerMatchers(MatchFinder *Finder) { const auto MakeBinaryOperatorMatcher = [](auto Op) { @@ -37,7 +39,10 @@ void UseStdBitCheck::registerMatchers(MatchFinder *Finder) { const auto LogicalAnd = MakeCommutativeBinaryOperatorMatcher("&&"); const auto Sub = MakeBinaryOperatorMatcher("-"); + const auto ShiftLeft = MakeBinaryOperatorMatcher("<<"); + const auto ShiftRight = MakeBinaryOperatorMatcher(">>"); const auto BitwiseAnd = MakeCommutativeBinaryOperatorMatcher("&"); + const auto BitwiseOr = MakeCommutativeBinaryOperatorMatcher("|"); const auto CmpNot = MakeCommutativeBinaryOperatorMatcher("!="); const auto CmpGt = MakeBinaryOperatorMatcher(">"); const auto CmpGte = MakeBinaryOperatorMatcher(">="); @@ -87,6 +92,16 @@ void UseStdBitCheck::registerMatchers(MatchFinder *Finder) { hasArgument(0, expr(hasType(isUnsignedInteger())).bind("v"))))) .bind("popcount_expr"), this); + + // Rotating an integer by a fixed amount + Finder->addMatcher( + expr(BitwiseOr(ShiftLeft(BindDeclRef("v"), + integerLiteral().bind("shift_left_amount")), + ShiftRight(BoundDeclRef("v"), + integerLiteral().bind("shift_right_amount"))), + optionally(hasParent(castExpr(hasType(isInteger())).bind("cast")))) + .bind("rotate_expr"), + this); } void UseStdBitCheck::registerPPCallbacks(const SourceManager &SM, @@ -97,10 +112,11 @@ void UseStdBitCheck::registerPPCallbacks(const SourceManager &SM, void UseStdBitCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle()); + Options.store(Opts, "HonorIntPromotion", HonorIntPromotion); } void UseStdBitCheck::check(const MatchFinder::MatchResult &Result) { - const ASTContext &Context = *Result.Context; + ASTContext &Context = *Result.Context; const SourceManager &Source = Context.getSourceManager(); if (const auto *MatchedExpr = @@ -141,6 +157,59 @@ void UseStdBitCheck::check(const MatchFinder::MatchResult &Result) { << IncludeInserter.createIncludeInsertion( Source.getFileID(MatchedExpr->getBeginLoc()), "<bit>"); } + } else if (const auto *MatchedExpr = + Result.Nodes.getNodeAs<Expr>("rotate_expr")) { + // Detect if the expression is an explicit cast. If that's the case we don't + // need to insert a cast. + + bool HasExplicitIntegerCast = false; + if (const Expr *CE = Result.Nodes.getNodeAs<CastExpr>("cast")) + HasExplicitIntegerCast = !isa<ImplicitCastExpr>(CE); + + const auto *MatchedVarDecl = Result.Nodes.getNodeAs<VarDecl>("v"); + const llvm::APInt ShiftLeftAmount = + Result.Nodes.getNodeAs<IntegerLiteral>("shift_left_amount")->getValue(); + const llvm::APInt ShiftRightAmount = + Result.Nodes.getNodeAs<IntegerLiteral>("shift_right_amount") + ->getValue(); + const uint64_t MatchedVarSize = + Context.getTypeSize(MatchedVarDecl->getType()); + + // Overflowing shifts + if (ShiftLeftAmount.sge(MatchedVarSize)) + return; + if (ShiftRightAmount.sge(MatchedVarSize)) + return; + // Not a rotation. + if (MatchedVarSize != (ShiftLeftAmount + ShiftRightAmount)) + return; + + // Only insert cast if the operand is not subject to cast and + // some implicit promotion happened. + const bool NeedsIntCast = + HonorIntPromotion && !HasExplicitIntegerCast && + Context.getTypeSize(MatchedExpr->getType()) > MatchedVarSize; + const bool IsRotl = ShiftRightAmount.sge(ShiftLeftAmount); + + const StringRef ReplacementFuncName = IsRotl ? "rotl" : "rotr"; + const uint64_t ReplacementShiftAmount = + (IsRotl ? ShiftLeftAmount : ShiftRightAmount).getZExtValue(); + auto Diag = diag(MatchedExpr->getBeginLoc(), "use 'std::%0' instead") + << ReplacementFuncName; + if (auto R = MatchedExpr->getSourceRange(); + R.getBegin().isMacroID() || R.getEnd().isMacroID()) + return; + + Diag << FixItHint::CreateReplacement( + MatchedExpr->getSourceRange(), + llvm::formatv("{3}std::{0}({1}, {2}){4}", ReplacementFuncName, + MatchedVarDecl->getName(), ReplacementShiftAmount, + NeedsIntCast ? "static_cast<int>(" : "", + NeedsIntCast ? ")" : "") + .str()) + << IncludeInserter.createIncludeInsertion( + Source.getFileID(MatchedExpr->getBeginLoc()), "<bit>"); + } else { llvm_unreachable("unexpected match"); } diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.h b/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.h index 6dd455d4286c4..abc76dc8c0d5b 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseStdBitCheck.h @@ -37,6 +37,7 @@ class UseStdBitCheck : public ClangTidyCheck { private: utils::IncludeInserter IncludeInserter; + const bool HonorIntPromotion; }; } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-bit.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-bit.rst index 26ef1b0841654..36848b7e3d228 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-bit.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-bit.rst @@ -15,4 +15,31 @@ Expression Replacement ``(x != 0) && !(x & (x - 1))`` ``std::has_one_bit(x)`` ``(x > 0) && !(x & (x - 1))`` ``std::has_one_bit(x)`` ``std::bitset<N>(x).count()`` ``std::popcount(x)`` +``x << 3 | x >> 61`` ``std::rotl(x, 3)`` +``x << 61 | x >> 3`` ``std::rotr(x, 3)`` ============================== ======================= + +Options +------- + +.. option:: HonorIntPromotion + + When set to ``true`` (default is ``false``), insert explicit cast to make sure the + type of the substituted expression is unchanged. Example: + + .. code:: c++ + + // Return type is deduced as 'int' (not 'unsigned char') due to implicit conversions. + auto foo(unsigned char x) { + return x << 3 | x >> 5; + } + + Becomes: + + .. code:: c++ + + #include <bit> + + auto foo(unsigned char x) { + return static_cast<int>(std::rotl(x, 3)); + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-bit.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-bit.cpp index 52c725282f33c..615d1202e8092 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-bit.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-bit.cpp @@ -1,4 +1,5 @@ -// RUN: %check_clang_tidy -std=c++20-or-later %s modernize-use-std-bit %t +// RUN: %check_clang_tidy -std=c++20-or-later %s modernize-use-std-bit %t -check-suffixes=,NOPROMOTION +// RUN: %check_clang_tidy -std=c++20-or-later %s modernize-use-std-bit %t -config="{CheckOptions: { modernize-use-std-bit.HonorIntPromotion: true }}" -check-suffixes=,PROMOTION // CHECK-FIXES: #include <bit> /* @@ -195,3 +196,109 @@ unsigned invalid_popcount_bitset(unsigned x, signed y) { }; } + +/* + * rotate patterns + */ + +using uint64_t = __UINT64_TYPE__; +using uint32_t = __UINT32_TYPE__; + +int rotate_left_pattern(unsigned char x) { + // CHECK-MESSAGES: :[[@LINE+3]]:10: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES-NOPROMOTION: return std::rotl(x, 3); + // CHECK-FIXES-PROMOTION: return static_cast<int>(std::rotl(x, 3)); + return (x) << 3 | x >> 5; +} + +auto rotate_left_pattern_with_cast(unsigned char x) { + // CHECK-MESSAGES: :[[@LINE+2]]:29: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES: return static_cast<short>(std::rotl(x, 3)); + return static_cast<short>((x) << 3 | x >> 5); +} + +unsigned char rotate_left_pattern_with_implicit_cast(unsigned char x) { + // CHECK-MESSAGES: :[[@LINE+3]]:10: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES-NOPROMOTION: return std::rotl(x, 3); + // CHECK-FIXES-PROMOTION: return static_cast<int>(std::rotl(x, 3)); + return (x) << 3 | x >> 5; +} + +auto rotate_left_pattern_without_cast(unsigned char x) { + // CHECK-MESSAGES: :[[@LINE+3]]:10: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES-NOPROMOTION: return std::rotl(x, 3); + // CHECK-FIXES-PROMOTION: return static_cast<int>(std::rotl(x, 3)); + return x << 3 | x >> 5; +} + +uint32_t rotate_left_pattern_with_surrounding_parenthesis(unsigned char x) { + // CHECK-MESSAGES: :[[@LINE+3]]:11: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES-NOPROMOTION: return (std::rotl(x, 3)); + // CHECK-FIXES-PROMOTION: return (static_cast<int>(std::rotl(x, 3))); + return (x << 3 | x >> 5); +} + +uint64_t rotate_left_pattern_int64(uint64_t x) { + // CHECK-MESSAGES: :[[@LINE+2]]:10: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES: return std::rotl(x, 3); + return x << 3 | x >> 61; +} + +uint32_t rotate_left_pattern_int32(uint32_t x) { + // CHECK-MESSAGES: :[[@LINE+2]]:10: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES: return std::rotl(x, 3); + return (x) << 3 | x >> 29; +} + +unsigned char rotate_left_pattern_perm(unsigned char x) { + // CHECK-MESSAGES: :[[@LINE+3]]:10: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES-NOPROMOTION: return std::rotl(x, 3); + // CHECK-FIXES-PROMOTION: return static_cast<int>(std::rotl(x, 3)); + return x >> 5 | x << 3; +} + +uint32_t rotate_swap_pattern(uint32_t x) { + // CHECK-MESSAGES: :[[@LINE+2]]:10: warning: use 'std::rotl' instead [modernize-use-std-bit] + // CHECK-FIXES: return std::rotl(x, 16); + return x << 16 | x >> 16; +} + +uint64_t rotate_right_pattern(uint64_t x) { + // CHECK-MESSAGES: :[[@LINE+2]]:10: warning: use 'std::rotr' instead [modernize-use-std-bit] + // CHECK-FIXES: return std::rotr(x, 3); + return (x << 61) | ((x >> 3)); +} + +unsigned char rotate_right_pattern_perm(unsigned char x0) { + // CHECK-MESSAGES: :[[@LINE+3]]:10: warning: use 'std::rotr' instead [modernize-use-std-bit] + // CHECK-FIXES-NOPROMOTION: return std::rotr(x0, 3); + // CHECK-FIXES-PROMOTION: return static_cast<int>(std::rotr(x0, 3)); + return x0 >> 3 | x0 << 5; +} + +#define ROTR v >> 3 | v << 5 +unsigned char rotate_macro(unsigned char v) { + // CHECK-MESSAGES: :[[@LINE+2]]:10: warning: use 'std::rotr' instead [modernize-use-std-bit] + // No fixes, it comes from macro expansion. + return ROTR; +} + +/* + * Invalid rotate patterns + */ +void invalid_rotate_patterns(unsigned char x, signed char y, unsigned char z) { + int patterns[] = { + // non-matching references + x >> 3 | z << 5, + // bad shift combination + x >> 3 | x << 6, + x >> 4 | x << 3, + // bad operator combination + x << 3 | x << 6, + x + 3 | x << 6, + x >> 3 & x << 5, + x >> 5 ^ x << 3, + // unsupported types + y >> 4 | y << 4, + }; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
