diff --git a/bolt/lib/Core/BinarySection.cpp b/bolt/lib/Core/BinarySection.cpp index 1e28da4cb2f7b..846515c3ed131 100644 --- a/bolt/lib/Core/BinarySection.cpp +++ b/bolt/lib/Core/BinarySection.cpp @@ -39,7 +39,13 @@ BinarySection::hash(const BinaryData &BD, if (Itr != Cache.end()) return Itr->second; - Cache[&BD] = 0; + hash_code Hash = + hash_combine(hash_value(BD.getSize()), hash_value(BD.getSectionName())); + + Cache[&BD] = Hash; + + if (!containsRange(BD.getAddress(), BD.getSize())) + return Hash; uint64_t Offset = BD.getAddress() - getAddress(); const uint64_t EndOffset = BD.getEndAddress() - getAddress(); @@ -47,9 +53,6 @@ BinarySection::hash(const BinaryData &BD, auto End = Relocations.upper_bound(Relocation{EndOffset, 0, 0, 0, 0}); const StringRef Contents = getContents(); - hash_code Hash = - hash_combine(hash_value(BD.getSize()), hash_value(BD.getSectionName())); - while (Begin != End) { const Relocation &Rel = *Begin++; Hash = hash_combine( diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp index 04ccbcf20de11..57e41d5b5724d 100644 --- a/bolt/lib/Rewrite/RewriteInstance.cpp +++ b/bolt/lib/Rewrite/RewriteInstance.cpp @@ -1050,6 +1050,16 @@ void RewriteInstance::discoverFileObjects() { LLVM_DEBUG(dbgs() << "BOLT-DEBUG: considering symbol " << UniqueName << " for function\n"); + if (Address == Section->getAddress() + Section->getSize()) { + assert(SymbolSize == 0 && + "unexpect non-zero sized symbol at end of section"); + LLVM_DEBUG( + dbgs() + << "BOLT-DEBUG: rejecting as symbol points to end of its section\n"); + registerName(SymbolSize); + continue; + } + if (!Section->isText()) { assert(SymbolType != SymbolRef::ST_Function && "unexpected function inside non-code section"); diff --git a/bolt/test/AArch64/Inputs/symbol-hashes.yaml b/bolt/test/AArch64/Inputs/symbol-hashes.yaml new file mode 100644 index 0000000000000..ce7f5928187fa --- /dev/null +++ b/bolt/test/AArch64/Inputs/symbol-hashes.yaml @@ -0,0 +1,83 @@ +--- !ELF +FileHeader: + Class: ELFCLASS64 + Data: ELFDATA2LSB + Type: ET_EXEC + Machine: EM_AARCH64 + Entry: 0x90 +ProgramHeaders: + - Type: PT_LOAD + Flags: [ PF_X, PF_R ] + FirstSec: .rodata + LastSec: .text + Align: 0x10000 + Offset: 0x0 +Sections: + - Name: .rodata + Type: SHT_PROGBITS + Flags: [ SHF_ALLOC ] + Address: 0x78 + AddressAlign: 0x1 + Content: '7800000000000000' + - Name: .dummy + Type: SHT_PROGBITS + Flags: [ SHF_ALLOC ] + Address: 0x80 + AddressAlign: 0x1 + Content: '78000000000000009000000000000000' + - Name: .text + Type: SHT_PROGBITS + Flags: [ SHF_ALLOC, SHF_EXECINSTR ] + Address: 0x90 + AddressAlign: 0x4 + Content: FF4300D11F2003D508FFFF10080140F9E80700F9A80B8052010000D4FF430091C0035FD6 + - Name: .rela.text + Type: SHT_RELA + Flags: [ SHF_INFO_LINK ] + Link: .symtab + AddressAlign: 0x8 + Info: .text + Relocations: + - Offset: 0x94 + Symbol: Symbol + Type: R_AARCH64_ADR_GOT_PAGE + - Offset: 0x98 + Symbol: Symbol + Type: R_AARCH64_LD64_GOT_LO12_NC + - Name: .rela.dummy + Type: SHT_RELA + Flags: [ SHF_INFO_LINK ] + Link: .symtab + AddressAlign: 0x8 + Info: .dummy + Relocations: + - Offset: 0x80 + Symbol: Symbol + Type: R_AARCH64_ABS64 + - Offset: 0x88 + Symbol: _start + Type: R_AARCH64_ABS64 +Symbols: + - Name: tmp.c + Type: STT_FILE + Index: SHN_ABS + - Name: '$x.0' + Section: .text + Value: 0x90 + - Name: '$d.1' + Index: SHN_ABS + - Name: .text + Type: STT_SECTION + Section: .text + Value: 0x90 + - Name: _start + Type: STT_FUNC + Section: .text + Binding: STB_GLOBAL + Value: 0x90 + Size: 0x24 + - Name: Symbol + Section: .rodata + Binding: STB_GLOBAL + Value: 0x78 +... diff --git a/bolt/test/AArch64/symbol-hashes.test b/bolt/test/AArch64/symbol-hashes.test new file mode 100644 index 0000000000000..3f2d869846c22 --- /dev/null +++ b/bolt/test/AArch64/symbol-hashes.test @@ -0,0 +1,6 @@ +// This test checks that we don't try to use symbols outside of section when +// generating symbol hashes + +RUN: yaml2obj %p/Inputs/symbol-hashes.yaml -o %t.exe +RUN: llvm-bolt %t.exe -force-data-relocations -o %t.exe.bolt + diff --git a/bolt/test/X86/section-end-sym.s b/bolt/test/X86/section-end-sym.s new file mode 100644 index 0000000000000..38517bf7e0719 --- /dev/null +++ b/bolt/test/X86/section-end-sym.s @@ -0,0 +1,29 @@ +## Check that BOLT doesn't consider end-of-section symbols (e.g., _etext) as +## functions. + +# REQUIRES: system-linux, asserts + +# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-linux %s -o %t.o +# RUN: ld.lld %t.o -o %t.exe -q +# RUN: llvm-bolt %t.exe -o /dev/null --print-cfg --debug-only=bolt 2>&1 \ +# RUN: | FileCheck %s + +# CHECK: considering symbol etext for function +# CHECK-NEXT: rejecting as symbol points to end of its section +# CHECK-NOT: Binary Function "etext{{.*}}" after building cfg + + + .text + .globl _start + .type _start,@function +_start: + retq + .size _start, .-_start + + .align 0x1000 + .globl etext +etext: + + .data +.Lfoo: + .word 0 diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index b7eadb87b4fcd..c10c3652a153a 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -58,11 +58,11 @@ class UseAfterMoveFinder { public: UseAfterMoveFinder(ASTContext *TheContext); - // Within the given function body, finds the first use of 'MovedVariable' that + // Within the given code block, finds the first use of 'MovedVariable' that // occurs after 'MovingCall' (the expression that performs the move). If a // use-after-move is found, writes information about it to 'TheUseAfterMove'. // Returns whether a use-after-move was found. - bool find(Stmt *FunctionBody, const Expr *MovingCall, + bool find(Stmt *CodeBlock, const Expr *MovingCall, const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove); private: @@ -104,7 +104,7 @@ static StatementMatcher inDecltypeOrTemplateArg() { UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext) : Context(TheContext) {} -bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall, +bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall, const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove) { // Generate the CFG manually instead of through an AnalysisDeclContext because @@ -118,12 +118,11 @@ bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall, Options.AddImplicitDtors = true; Options.AddTemporaryDtors = true; std::unique_ptr TheCFG = - CFG::buildCFG(nullptr, FunctionBody, Context, Options); + CFG::buildCFG(nullptr, CodeBlock, Context, Options); if (!TheCFG) return false; - Sequence = - std::make_unique(TheCFG.get(), FunctionBody, Context); + Sequence = std::make_unique(TheCFG.get(), CodeBlock, Context); BlockMap = std::make_unique(TheCFG.get(), Context); Visited.clear(); @@ -398,20 +397,28 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, } void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { + // try_emplace is a common maybe-moving function that returns a + // bool to tell callers whether it moved. Ignore std::move inside + // try_emplace to avoid false positives as we don't track uses of + // the bool. + auto TryEmplaceMatcher = + cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace")))); auto CallMoveMatcher = - callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1), + callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))), hasArgument(0, declRefExpr().bind("arg")), + unless(inDecltypeOrTemplateArg()), + unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"), anyOf(hasAncestor(compoundStmt( hasParent(lambdaExpr().bind("containing-lambda")))), - hasAncestor(functionDecl().bind("containing-func"))), - unless(inDecltypeOrTemplateArg()), - // try_emplace is a common maybe-moving function that returns a - // bool to tell callers whether it moved. Ignore std::move inside - // try_emplace to avoid false positives as we don't track uses of - // the bool. - unless(hasParent(cxxMemberCallExpr( - callee(cxxMethodDecl(hasName("try_emplace"))))))) - .bind("call-move"); + hasAncestor(functionDecl(anyOf( + cxxConstructorDecl( + hasAnyConstructorInitializer(withInitializer( + expr(anyOf(equalsBoundNode("call-move"), + hasDescendant(expr( + equalsBoundNode("call-move"))))) + .bind("containing-ctor-init")))) + .bind("containing-ctor"), + functionDecl().bind("containing-func")))))); Finder->addMatcher( traverse( @@ -434,6 +441,10 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { } void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { + const auto *ContainingCtor = + Result.Nodes.getNodeAs("containing-ctor"); + const auto *ContainingCtorInit = + Result.Nodes.getNodeAs("containing-ctor-init"); const auto *ContainingLambda = Result.Nodes.getNodeAs("containing-lambda"); const auto *ContainingFunc = @@ -445,23 +456,38 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { if (!MovingCall || !MovingCall->getExprLoc().isValid()) MovingCall = CallMove; - Stmt *FunctionBody = nullptr; - if (ContainingLambda) - FunctionBody = ContainingLambda->getBody(); - else if (ContainingFunc) - FunctionBody = ContainingFunc->getBody(); - else - return; - // Ignore the std::move if the variable that was passed to it isn't a local // variable. if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod()) return; - UseAfterMoveFinder Finder(Result.Context); - UseAfterMove Use; - if (Finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use)) - emitDiagnostic(MovingCall, Arg, Use, this, Result.Context); + // Collect all code blocks that could use the arg after move. + llvm::SmallVector CodeBlocks{}; + if (ContainingCtor) { + CodeBlocks.push_back(ContainingCtor->getBody()); + if (ContainingCtorInit) { + // Collect the constructor initializer expressions. + bool BeforeMove{true}; + for (CXXCtorInitializer *Init : ContainingCtor->inits()) { + if (BeforeMove && Init->getInit()->IgnoreImplicit() == + ContainingCtorInit->IgnoreImplicit()) + BeforeMove = false; + if (!BeforeMove) + CodeBlocks.push_back(Init->getInit()); + } + } + } else if (ContainingLambda) { + CodeBlocks.push_back(ContainingLambda->getBody()); + } else if (ContainingFunc) { + CodeBlocks.push_back(ContainingFunc->getBody()); + } + + for (Stmt *CodeBlock : CodeBlocks) { + UseAfterMoveFinder Finder(Result.Context); + UseAfterMove Use; + if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use)) + emitDiagnostic(MovingCall, Arg, Use, this, Result.Context); + } } } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp b/clang-tools-extra/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp index c5bd6055072aa..d522d6760af1d 100644 --- a/clang-tools-extra/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp +++ b/clang-tools-extra/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp @@ -47,16 +47,19 @@ class AvoidUnderscoreInGoogletestNameCallback : public PPCallbacks { if (!isGoogletestTestMacro(MacroName) || !Args || Args->getNumMacroArguments() < 2) return; - const Token *TestCaseNameToken = Args->getUnexpArgument(0); + const Token *TestSuiteNameToken = Args->getUnexpArgument(0); const Token *TestNameToken = Args->getUnexpArgument(1); - if (!TestCaseNameToken || !TestNameToken) + if (!TestSuiteNameToken || !TestNameToken) return; - std::string TestCaseName = PP->getSpelling(*TestCaseNameToken); - if (TestCaseName.find('_') != std::string::npos) - Check->diag(TestCaseNameToken->getLocation(), - "avoid using \"_\" in test case name \"%0\" according to " + std::string TestSuiteNameMaybeDisabled = + PP->getSpelling(*TestSuiteNameToken); + StringRef TestSuiteName = TestSuiteNameMaybeDisabled; + TestSuiteName.consume_front(KDisabledTestPrefix); + if (TestSuiteName.contains('_')) + Check->diag(TestSuiteNameToken->getLocation(), + "avoid using \"_\" in test suite name \"%0\" according to " "Googletest FAQ") - << TestCaseName; + << TestSuiteName; std::string TestNameMaybeDisabled = PP->getSpelling(*TestNameToken); StringRef TestName = TestNameMaybeDisabled; diff --git a/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp index d48d43f2677f0..afac9f7497ac8 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp @@ -235,19 +235,19 @@ void UseEqualsDefaultCheck::registerMatchers(MatchFinder *Finder) { // Destructor. Finder->addMatcher( - cxxDestructorDecl(unless(hasParent(IsUnionLikeClass)), isDefinition()) + cxxDestructorDecl(isDefinition(), unless(ofClass(IsUnionLikeClass))) .bind(SpecialFunction), this); + // Constructor. Finder->addMatcher( cxxConstructorDecl( - unless( - hasParent(decl(anyOf(IsUnionLikeClass, functionTemplateDecl())))), - isDefinition(), + isDefinition(), unless(ofClass(IsUnionLikeClass)), + unless(hasParent(functionTemplateDecl())), anyOf( // Default constructor. - allOf(unless(hasAnyConstructorInitializer(isWritten())), - unless(isVariadic()), parameterCountIs(0), - IsPublicOrOutOfLineUntilCPP20), + allOf(parameterCountIs(0), + unless(hasAnyConstructorInitializer(isWritten())), + unless(isVariadic()), IsPublicOrOutOfLineUntilCPP20), // Copy constructor. allOf(isCopyConstructor(), // Discard constructors that can be used as a copy @@ -258,9 +258,9 @@ void UseEqualsDefaultCheck::registerMatchers(MatchFinder *Finder) { this); // Copy-assignment operator. Finder->addMatcher( - cxxMethodDecl(unless(hasParent( - decl(anyOf(IsUnionLikeClass, functionTemplateDecl())))), - isDefinition(), isCopyAssignmentOperator(), + cxxMethodDecl(isDefinition(), isCopyAssignmentOperator(), + unless(ofClass(IsUnionLikeClass)), + unless(hasParent(functionTemplateDecl())), // isCopyAssignmentOperator() allows the parameter to be // passed by value, and in this case it cannot be // defaulted. @@ -299,6 +299,12 @@ void UseEqualsDefaultCheck::check(const MatchFinder::MatchResult &Result) { if (!SpecialFunctionDecl->isCopyAssignmentOperator() && !Body->body_empty()) return; + // If body contain any preprocesor derictives, don't warn. + if (IgnoreMacros && utils::lexer::rangeContainsExpansionsOrDirectives( + Body->getSourceRange(), *Result.SourceManager, + Result.Context->getLangOpts())) + return; + // If there are comments inside the body, don't do the change. bool ApplyFix = SpecialFunctionDecl->isCopyAssignmentOperator() || bodyEmpty(Result.Context, Body); diff --git a/clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp new file mode 100644 index 0000000000000..d92d0e8f2dbf7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp @@ -0,0 +1,104 @@ +//===--- AvoidUnconditionalPreprocessorIfCheck.cpp - clang-tidy -----------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "AvoidUnconditionalPreprocessorIfCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { +struct AvoidUnconditionalPreprocessorIfPPCallbacks : public PPCallbacks { + + explicit AvoidUnconditionalPreprocessorIfPPCallbacks(ClangTidyCheck &Check, + Preprocessor &PP) + : Check(Check), PP(PP) {} + + void If(SourceLocation Loc, SourceRange ConditionRange, + ConditionValueKind ConditionValue) override { + if (ConditionValue == CVK_NotEvaluated) + return; + SourceManager &SM = PP.getSourceManager(); + if (!isImmutable(SM, PP.getLangOpts(), ConditionRange)) + return; + + if (ConditionValue == CVK_True) + Check.diag(Loc, "preprocessor condition is always 'true', consider " + "removing condition but leaving its contents"); + else + Check.diag(Loc, "preprocessor condition is always 'false', consider " + "removing both the condition and its contents"); + } + + bool isImmutable(SourceManager &SM, const LangOptions &LangOpts, + SourceRange ConditionRange) { + SourceLocation Loc = ConditionRange.getBegin(); + if (Loc.isMacroID()) + return false; + + Token Tok; + if (Lexer::getRawToken(Loc, Tok, SM, LangOpts, true)) { + std::optional TokOpt = Lexer::findNextToken(Loc, SM, LangOpts); + if (!TokOpt || TokOpt->getLocation().isMacroID()) + return false; + Tok = *TokOpt; + } + + while (Tok.getLocation() <= ConditionRange.getEnd()) { + if (!isImmutableToken(Tok)) + return false; + + std::optional TokOpt = + Lexer::findNextToken(Tok.getLocation(), SM, LangOpts); + if (!TokOpt || TokOpt->getLocation().isMacroID()) + return false; + Tok = *TokOpt; + } + + return true; + } + + bool isImmutableToken(const Token &Tok) { + switch (Tok.getKind()) { + case tok::eod: + case tok::eof: + case tok::numeric_constant: + case tok::char_constant: + case tok::wide_char_constant: + case tok::utf8_char_constant: + case tok::utf16_char_constant: + case tok::utf32_char_constant: + case tok::string_literal: + case tok::wide_string_literal: + case tok::comment: + return true; + case tok::raw_identifier: + return (Tok.getRawIdentifier() == "true" || + Tok.getRawIdentifier() == "false"); + default: + return Tok.getKind() >= tok::l_square && Tok.getKind() <= tok::caretcaret; + } + } + + ClangTidyCheck &Check; + Preprocessor &PP; +}; + +} // namespace + +void AvoidUnconditionalPreprocessorIfCheck::registerPPCallbacks( + const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { + PP->addPPCallbacks( + std::make_unique(*this, + *PP)); +} + +} // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.h b/clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.h new file mode 100644 index 0000000000000..50292fce9d8dc --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.h @@ -0,0 +1,32 @@ +//===--- AvoidUnconditionalPreprocessorIfCheck.h - clang-tidy ---*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDUNCONDITIONALPREPROCESSORIFCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDUNCONDITIONALPREPROCESSORIFCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::readability { + +/// Finds code blocks that are constantly enabled or disabled in preprocessor +/// directives by analyzing `#if` conditions, such as `#if 0` and `#if 1`, etc. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.html +class AvoidUnconditionalPreprocessorIfCheck : public ClangTidyCheck { +public: + AvoidUnconditionalPreprocessorIfCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDUNCONDITIONALPREPROCESSORIFCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index ea09b2193eb7c..2306641ca9215 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangTidyReadabilityModule AvoidConstParamsInDecls.cpp + AvoidUnconditionalPreprocessorIfCheck.cpp BracesAroundStatementsCheck.cpp ConstReturnTypeCheck.cpp ContainerContainsCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp index ca81b6e3c6e61..cffe858c39ad1 100644 --- a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp @@ -81,6 +81,8 @@ MagicNumbersCheck::MagicNumbersCheck(StringRef Name, ClangTidyContext *Context) IgnorePowersOf2IntegerValues( Options.get("IgnorePowersOf2IntegerValues", false)), IgnoreTypeAliases(Options.get("IgnoreTypeAliases", false)), + IgnoreUserDefinedLiterals( + Options.get("IgnoreUserDefinedLiterals", false)), RawIgnoredIntegerValues( Options.get("IgnoredIntegerValues", DefaultIgnoredIntegerValues)), RawIgnoredFloatingPointValues(Options.get( @@ -130,6 +132,7 @@ void MagicNumbersCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "IgnorePowersOf2IntegerValues", IgnorePowersOf2IntegerValues); Options.store(Opts, "IgnoreTypeAliases", IgnoreTypeAliases); + Options.store(Opts, "IgnoreUserDefinedLiterals", IgnoreUserDefinedLiterals); Options.store(Opts, "IgnoredIntegerValues", RawIgnoredIntegerValues); Options.store(Opts, "IgnoredFloatingPointValues", RawIgnoredFloatingPointValues); @@ -243,5 +246,14 @@ bool MagicNumbersCheck::isBitFieldWidth( }); } +bool MagicNumbersCheck::isUserDefinedLiteral( + const clang::ast_matchers::MatchFinder::MatchResult &Result, + const clang::Expr &Literal) const { + DynTypedNodeList Parents = Result.Context->getParents(Literal); + if (Parents.empty()) + return false; + return Parents[0].get() != nullptr; +} + } // namespace tidy::readability } // namespace clang diff --git a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h index 39e9baea3adc8..79787845de619 100644 --- a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h +++ b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h @@ -49,6 +49,10 @@ class MagicNumbersCheck : public ClangTidyCheck { bool isBitFieldWidth(const clang::ast_matchers::MatchFinder::MatchResult &Result, const IntegerLiteral &Literal) const; + bool isUserDefinedLiteral( + const clang::ast_matchers::MatchFinder::MatchResult &Result, + const clang::Expr &Literal) const; + template void checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result, const char *BoundName) { @@ -72,6 +76,10 @@ class MagicNumbersCheck : public ClangTidyCheck { if (isBitFieldWidth(Result, *MatchedLiteral)) return; + if (IgnoreUserDefinedLiterals && + isUserDefinedLiteral(Result, *MatchedLiteral)) + return; + const StringRef LiteralSourceText = Lexer::getSourceText( CharSourceRange::getTokenRange(MatchedLiteral->getSourceRange()), *Result.SourceManager, getLangOpts()); @@ -85,6 +93,7 @@ class MagicNumbersCheck : public ClangTidyCheck { const bool IgnoreBitFieldsWidths; const bool IgnorePowersOf2IntegerValues; const bool IgnoreTypeAliases; + const bool IgnoreUserDefinedLiterals; const StringRef RawIgnoredIntegerValues; const StringRef RawIgnoredFloatingPointValues; diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index 7aa090424257d..7fd9191844897 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "AvoidConstParamsInDecls.h" +#include "AvoidUnconditionalPreprocessorIfCheck.h" #include "BracesAroundStatementsCheck.h" #include "ConstReturnTypeCheck.h" #include "ContainerContainsCheck.h" @@ -60,6 +61,8 @@ class ReadabilityModule : public ClangTidyModule { void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "readability-avoid-const-params-in-decls"); + CheckFactories.registerCheck( + "readability-avoid-unconditional-preprocessor-if"); CheckFactories.registerCheck( "readability-braces-around-statements"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clangd/CollectMacros.cpp b/clang-tools-extra/clangd/CollectMacros.cpp index 687f86e0a77eb..c0ed8b68ea481 100644 --- a/clang-tools-extra/clangd/CollectMacros.cpp +++ b/clang-tools-extra/clangd/CollectMacros.cpp @@ -9,12 +9,13 @@ #include "CollectMacros.h" #include "AST.h" #include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/STLExtras.h" namespace clang { namespace clangd { void CollectMainFileMacros::add(const Token &MacroNameTok, const MacroInfo *MI, - bool IsDefinition) { + bool IsDefinition, bool InIfCondition) { if (!InMainFile) return; auto Loc = MacroNameTok.getLocation(); @@ -26,9 +27,49 @@ void CollectMainFileMacros::add(const Token &MacroNameTok, const MacroInfo *MI, auto Range = halfOpenToRange( SM, CharSourceRange::getCharRange(Loc, MacroNameTok.getEndLoc())); if (auto SID = getSymbolID(Name, MI, SM)) - Out.MacroRefs[SID].push_back({Range, IsDefinition}); + Out.MacroRefs[SID].push_back({Range, IsDefinition, InIfCondition}); else - Out.UnknownMacros.push_back({Range, IsDefinition}); + Out.UnknownMacros.push_back({Range, IsDefinition, InIfCondition}); +} + +void CollectMainFileMacros::FileChanged(SourceLocation Loc, FileChangeReason, + SrcMgr::CharacteristicKind, FileID) { + InMainFile = isInsideMainFile(Loc, SM); +} +void CollectMainFileMacros::MacroExpands(const Token &MacroName, + const MacroDefinition &MD, + SourceRange Range, + const MacroArgs *Args) { + add(MacroName, MD.getMacroInfo()); +} +void CollectMainFileMacros::MacroUndefined(const clang::Token &MacroName, + const clang::MacroDefinition &MD, + const clang::MacroDirective *Undef) { + add(MacroName, MD.getMacroInfo()); +} +void CollectMainFileMacros::Ifdef(SourceLocation Loc, const Token &MacroName, + const MacroDefinition &MD) { + add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false, + /*InConditionalDirective=*/true); +} +void CollectMainFileMacros::Ifndef(SourceLocation Loc, const Token &MacroName, + const MacroDefinition &MD) { + add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false, + /*InConditionalDirective=*/true); +} +void CollectMainFileMacros::Defined(const Token &MacroName, + const MacroDefinition &MD, + SourceRange Range) { + add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false, + /*InConditionalDirective=*/true); +} +void CollectMainFileMacros::SourceRangeSkipped(SourceRange R, + SourceLocation EndifLoc) { + if (!InMainFile) + return; + Position Begin = sourceLocToPosition(SM, R.getBegin()); + Position End = sourceLocToPosition(SM, R.getEnd()); + Out.SkippedRanges.push_back(Range{Begin, End}); } class CollectPragmaMarks : public PPCallbacks { @@ -58,5 +99,24 @@ collectPragmaMarksCallback(const SourceManager &SM, return std::make_unique(SM, Out); } +void CollectMainFileMacros::MacroDefined(const Token &MacroName, + const MacroDirective *MD) { + + if (!InMainFile) + return; + const auto *MI = MD->getMacroInfo(); + add(MacroName, MD->getMacroInfo(), true); + if (MI) + for (const auto &Tok : MI->tokens()) { + auto *II = Tok.getIdentifierInfo(); + // Could this token be a reference to a macro? (Not param to this macro). + if (!II || !II->hadMacroDefinition() || + llvm::is_contained(MI->params(), II)) + continue; + if (const MacroInfo *MI = PP.getMacroInfo(II)) + add(Tok, MI); + } +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/CollectMacros.h b/clang-tools-extra/clangd/CollectMacros.h index 9d7b478f1c3c7..d5789a2a88912 100644 --- a/clang-tools-extra/clangd/CollectMacros.h +++ b/clang-tools-extra/clangd/CollectMacros.h @@ -13,6 +13,7 @@ #include "SourceCode.h" #include "index/SymbolID.h" #include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" #include "llvm/ADT/DenseMap.h" #include @@ -24,6 +25,8 @@ struct MacroOccurrence { // SourceManager from preamble is not available when we build the AST. Range Rng; bool IsDefinition; + // True if the occurence is used in a conditional directive, e.g. #ifdef MACRO + bool InConditionalDirective; }; struct MainFileMacros { @@ -43,56 +46,37 @@ struct MainFileMacros { /// - collect macros after the preamble of the main file (in ParsedAST.cpp) class CollectMainFileMacros : public PPCallbacks { public: - explicit CollectMainFileMacros(const SourceManager &SM, MainFileMacros &Out) - : SM(SM), Out(Out) {} + explicit CollectMainFileMacros(const Preprocessor &PP, MainFileMacros &Out) + : SM(PP.getSourceManager()), PP(PP), Out(Out) {} void FileChanged(SourceLocation Loc, FileChangeReason, - SrcMgr::CharacteristicKind, FileID) override { - InMainFile = isInsideMainFile(Loc, SM); - } + SrcMgr::CharacteristicKind, FileID) override; - void MacroDefined(const Token &MacroName, const MacroDirective *MD) override { - add(MacroName, MD->getMacroInfo(), /*IsDefinition=*/true); - } + void MacroDefined(const Token &MacroName, const MacroDirective *MD) override; void MacroExpands(const Token &MacroName, const MacroDefinition &MD, - SourceRange Range, const MacroArgs *Args) override { - add(MacroName, MD.getMacroInfo()); - } + SourceRange Range, const MacroArgs *Args) override; void MacroUndefined(const clang::Token &MacroName, const clang::MacroDefinition &MD, - const clang::MacroDirective *Undef) override { - add(MacroName, MD.getMacroInfo()); - } + const clang::MacroDirective *Undef) override; + // FIXME: handle C++23 #elifdef, #elifndef void Ifdef(SourceLocation Loc, const Token &MacroName, - const MacroDefinition &MD) override { - add(MacroName, MD.getMacroInfo()); - } - + const MacroDefinition &MD) override; void Ifndef(SourceLocation Loc, const Token &MacroName, - const MacroDefinition &MD) override { - add(MacroName, MD.getMacroInfo()); - } + const MacroDefinition &MD) override; void Defined(const Token &MacroName, const MacroDefinition &MD, - SourceRange Range) override { - add(MacroName, MD.getMacroInfo()); - } - - void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override { - if (!InMainFile) - return; - Position Begin = sourceLocToPosition(SM, R.getBegin()); - Position End = sourceLocToPosition(SM, R.getEnd()); - Out.SkippedRanges.push_back(Range{Begin, End}); - } + SourceRange Range) override; + + void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override; private: void add(const Token &MacroNameTok, const MacroInfo *MI, - bool IsDefinition = false); + bool IsDefinition = false, bool InConditionalDirective = false); const SourceManager &SM; + const Preprocessor &PP; bool InMainFile = true; MainFileMacros &Out; }; diff --git a/clang-tools-extra/clangd/FindSymbols.cpp b/clang-tools-extra/clangd/FindSymbols.cpp index 2eab7ca27033e..790ee9af8f4ac 100644 --- a/clang-tools-extra/clangd/FindSymbols.cpp +++ b/clang-tools-extra/clangd/FindSymbols.cpp @@ -223,8 +223,8 @@ std::string getSymbolDetail(ASTContext &Ctx, const NamedDecl &ND) { std::optional declToSym(ASTContext &Ctx, const NamedDecl &ND) { auto &SM = Ctx.getSourceManager(); - SourceLocation BeginLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getBeginLoc())); - SourceLocation EndLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getEndLoc())); + SourceLocation BeginLoc = SM.getFileLoc(ND.getBeginLoc()); + SourceLocation EndLoc = SM.getFileLoc(ND.getEndLoc()); const auto SymbolRange = toHalfOpenFileRange(SM, Ctx.getLangOpts(), {BeginLoc, EndLoc}); if (!SymbolRange) diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp index c5436141adbf7..27663991a7f3c 100644 --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -12,11 +12,16 @@ #include "CodeCompletionStrings.h" #include "Config.h" #include "FindTarget.h" +#include "IncludeCleaner.h" #include "ParsedAST.h" #include "Selection.h" #include "SourceCode.h" +#include "clang-include-cleaner/Analysis.h" +#include "clang-include-cleaner/Types.h" #include "index/SymbolCollector.h" +#include "support/Logger.h" #include "support/Markup.h" +#include "support/Trace.h" #include "clang/AST/ASTContext.h" #include "clang/AST/ASTDiagnostic.h" #include "clang/AST/ASTTypeTraits.h" @@ -43,11 +48,13 @@ #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" +#include "llvm/Support/Error.h" #include "llvm/Support/Format.h" #include "llvm/Support/ScopedPrinter.h" #include "llvm/Support/raw_ostream.h" #include #include +#include namespace clang { namespace clangd { @@ -393,9 +400,9 @@ void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D, // 100 => 0x64 // Negative numbers are sign-extended to 32/64 bits // -2 => 0xfffffffe -// -2^32 => 0xfffffffeffffffff +// -2^32 => 0xffffffff00000000 static llvm::FormattedNumber printHex(const llvm::APSInt &V) { - uint64_t Bits = V.getExtValue(); + uint64_t Bits = V.getZExtValue(); if (V.isNegative() && V.getSignificantBits() <= 32) return llvm::format_hex(uint32_t(Bits), 0); return llvm::format_hex(Bits, 0); @@ -1084,6 +1091,49 @@ const NamedDecl *pickDeclToUse(llvm::ArrayRef Candidates) { return Candidates.front(); } +void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo &HI, + include_cleaner::Symbol Sym) { + trace::Span Tracer("Hover::maybeAddSymbolProviders"); + + const SourceManager &SM = AST.getSourceManager(); + llvm::SmallVector RankedProviders = + include_cleaner::headersForSymbol(Sym, SM, AST.getPragmaIncludes()); + if (RankedProviders.empty()) + return; + + std::string Result; + include_cleaner::Includes ConvertedIncludes = + convertIncludes(SM, AST.getIncludeStructure().MainFileIncludes); + for (const auto &P : RankedProviders) { + if (P.kind() == include_cleaner::Header::Physical && + P.physical() == SM.getFileEntryForID(SM.getMainFileID())) + // Main file ranked higher than any #include'd file + break; + + // Pick the best-ranked #include'd provider + auto Matches = ConvertedIncludes.match(P); + if (!Matches.empty()) { + Result = Matches[0]->quote(); + break; + } + } + + if (!Result.empty()) { + HI.Provider = std::move(Result); + return; + } + + // Pick the best-ranked non-#include'd provider + const auto &H = RankedProviders.front(); + if (H.kind() == include_cleaner::Header::Physical && + H.physical() == SM.getFileEntryForID(SM.getMainFileID())) + // Do not show main file as provider, otherwise we'll show provider info + // on local variables, etc. + return; + + HI.Provider = spellHeader(AST, SM.getFileEntryForID(SM.getMainFileID()), H); +} + } // namespace std::optional getHover(ParsedAST &AST, Position Pos, @@ -1131,6 +1181,12 @@ std::optional getHover(ParsedAST &AST, Position Pos, HighlightRange = Tok.range(SM).toCharRange(SM); if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) { HI = getHoverContents(*M, Tok, AST); + if (auto DefLoc = M->Info->getDefinitionLoc(); DefLoc.isValid()) { + include_cleaner::Macro IncludeCleanerMacro{ + AST.getPreprocessor().getIdentifierInfo(Tok.text(SM)), DefLoc}; + maybeAddSymbolProviders(AST, *HI, + include_cleaner::Symbol{IncludeCleanerMacro}); + } break; } } else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) { @@ -1168,6 +1224,7 @@ std::optional getHover(ParsedAST &AST, Position Pos, if (!HI->Value) HI->Value = printExprValue(N, AST.getASTContext()); maybeAddCalleeArgInfo(N, *HI, PP); + maybeAddSymbolProviders(AST, *HI, include_cleaner::Symbol{*DeclToUse}); } else if (const Expr *E = N->ASTNode.get()) { HI = getHoverContents(N, E, AST, PP, Index); } else if (const Attr *A = N->ASTNode.get()) { @@ -1217,6 +1274,14 @@ markup::Document HoverInfo::present() const { assert(!Name.empty() && "hover triggered on a nameless symbol"); Header.appendCode(Name); + if (!Provider.empty()) { + markup::Paragraph &DI = Output.addParagraph(); + DI.appendText("provided by"); + DI.appendSpace(); + DI.appendCode(Provider); + Output.addRuler(); + } + // Put a linebreak after header to increase readability. Output.addRuler(); // Print Types on their own lines to reduce chances of getting line-wrapped by diff --git a/clang-tools-extra/clangd/Hover.h b/clang-tools-extra/clangd/Hover.h index e63ff95b400b3..7ade177f89cc1 100644 --- a/clang-tools-extra/clangd/Hover.h +++ b/clang-tools-extra/clangd/Hover.h @@ -14,6 +14,7 @@ #include "support/Markup.h" #include "clang/Index/IndexSymbol.h" #include +#include namespace clang { namespace clangd { @@ -67,6 +68,8 @@ struct HoverInfo { std::string LocalScope; /// Name of the symbol, does not contain any "::". std::string Name; + /// Header providing the symbol (best match). Contains ""<>. + std::string Provider; std::optional SymRange; index::SymbolKind Kind = index::SymbolKind::Unknown; std::string Documentation; diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp index 98135529f259b..ab7c05eb834c0 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -93,8 +93,6 @@ bool isFilteredByConfig(const Config &Cfg, llvm::StringRef HeaderPath) { static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST, const Config &Cfg, const include_cleaner::PragmaIncludes *PI) { - if (PI && PI->shouldKeep(Inc.HashLine + 1)) - return false; // FIXME(kirillbobyrev): We currently do not support the umbrella headers. // System headers are likely to be standard library headers. // Until we have good support for umbrella headers, don't warn about them. @@ -108,6 +106,20 @@ static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST, auto FE = AST.getSourceManager().getFileManager().getFileRef( AST.getIncludeStructure().getRealPath(HID)); assert(FE); + if (PI) { + if (PI->shouldKeep(Inc.HashLine + 1)) + return false; + // Check if main file is the public interface for a private header. If so we + // shouldn't diagnose it as unused. + if(auto PHeader = PI->getPublic(*FE); !PHeader.empty()) { + PHeader = PHeader.trim("<>\""); + // Since most private -> public mappings happen in a verbatim way, we + // check textually here. This might go wrong in presence of symlinks or + // header mappings. But that's not different than rest of the places. + if(AST.tuPath().endswith(PHeader)) + return false; + } + } // Headers without include guards have side effects and are not // self-contained, skip them. if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded( @@ -124,45 +136,6 @@ static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST, return true; } -include_cleaner::Includes -convertIncludes(const SourceManager &SM, - const llvm::ArrayRef MainFileIncludes) { - include_cleaner::Includes Includes; - for (const Inclusion &Inc : MainFileIncludes) { - include_cleaner::Include TransformedInc; - llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written); - TransformedInc.Spelled = WrittenRef.trim("\"<>"); - TransformedInc.HashLocation = - SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset); - TransformedInc.Line = Inc.HashLine + 1; - TransformedInc.Angled = WrittenRef.starts_with("<"); - auto FE = SM.getFileManager().getFile(Inc.Resolved); - if (!FE) { - elog("IncludeCleaner: Failed to get an entry for resolved path {0}: {1}", - Inc.Resolved, FE.getError().message()); - continue; - } - TransformedInc.Resolved = *FE; - Includes.add(std::move(TransformedInc)); - } - return Includes; -} - -std::string spellHeader(ParsedAST &AST, const FileEntry *MainFile, - include_cleaner::Header Provider) { - if (Provider.kind() == include_cleaner::Header::Physical) { - if (auto CanonicalPath = - getCanonicalPath(Provider.physical(), AST.getSourceManager())) { - std::string SpelledHeader = - llvm::cantFail(URI::includeSpelling(URI::create(*CanonicalPath))); - if (!SpelledHeader.empty()) - return SpelledHeader; - } - } - return include_cleaner::spellHeader( - Provider, AST.getPreprocessor().getHeaderSearchInfo(), MainFile); -} - std::vector collectMacroReferences(ParsedAST &AST) { const auto &SM = AST.getSourceManager(); @@ -315,6 +288,44 @@ std::vector generateUnusedIncludeDiagnostics( } } // namespace +include_cleaner::Includes +convertIncludes(const SourceManager &SM, + const llvm::ArrayRef Includes) { + include_cleaner::Includes ConvertedIncludes; + for (const Inclusion &Inc : Includes) { + include_cleaner::Include TransformedInc; + llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written); + TransformedInc.Spelled = WrittenRef.trim("\"<>"); + TransformedInc.HashLocation = + SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset); + TransformedInc.Line = Inc.HashLine + 1; + TransformedInc.Angled = WrittenRef.starts_with("<"); + auto FE = SM.getFileManager().getFile(Inc.Resolved); + if (!FE) { + elog("IncludeCleaner: Failed to get an entry for resolved path {0}: {1}", + Inc.Resolved, FE.getError().message()); + continue; + } + TransformedInc.Resolved = *FE; + ConvertedIncludes.add(std::move(TransformedInc)); + } + return ConvertedIncludes; +} + +std::string spellHeader(ParsedAST &AST, const FileEntry *MainFile, + include_cleaner::Header Provider) { + if (Provider.kind() == include_cleaner::Header::Physical) { + if (auto CanonicalPath = + getCanonicalPath(Provider.physical(), AST.getSourceManager())) { + std::string SpelledHeader = + llvm::cantFail(URI::includeSpelling(URI::create(*CanonicalPath))); + if (!SpelledHeader.empty()) + return SpelledHeader; + } + } + return include_cleaner::spellHeader( + Provider, AST.getPreprocessor().getHeaderSearchInfo(), MainFile); +} std::vector getUnused(ParsedAST &AST, diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h index d7edca035c965..1a5f07869d569 100644 --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -68,6 +68,16 @@ std::vector issueIncludeCleanerDiagnostics(ParsedAST &AST, /// FIXME: remove this hack once the implementation is good enough. void setIncludeCleanerAnalyzesStdlib(bool B); +/// Converts the clangd include representation to include-cleaner +/// include representation. +include_cleaner::Includes +convertIncludes(const SourceManager &SM, + const llvm::ArrayRef Includes); + +/// Determines the header spelling of an include-cleaner header +/// representation. The spelling contains the ""<> characters. +std::string spellHeader(ParsedAST &AST, const FileEntry *MainFile, + include_cleaner::Header Provider); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 1671eec133b6e..1501a5c5f3c3b 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -610,11 +610,12 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, Macros = Patch->mainFileMacros(); Marks = Patch->marks(); } - Clang->getPreprocessor().addPPCallbacks( - std::make_unique(Clang->getSourceManager(), - Macros)); + auto& PP = Clang->getPreprocessor(); + PP.addPPCallbacks( + std::make_unique( + PP, Macros)); - Clang->getPreprocessor().addPPCallbacks( + PP.addPPCallbacks( collectPragmaMarksCallback(Clang->getSourceManager(), Marks)); // Copy over the includes from the preamble, then combine with the @@ -626,10 +627,10 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, CanonIncludes.addSystemHeadersMapping(Clang->getLangOpts()); std::unique_ptr IWYUHandler = collectIWYUHeaderMaps(&CanonIncludes); - Clang->getPreprocessor().addCommentHandler(IWYUHandler.get()); + PP.addCommentHandler(IWYUHandler.get()); // Collect tokens of the main file. - syntax::TokenCollector CollectTokens(Clang->getPreprocessor()); + syntax::TokenCollector CollectTokens(PP); // To remain consistent with preamble builds, these callbacks must be called // exactly here, after preprocessor is initialized and BeginSourceFile() was @@ -660,7 +661,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF. // However Action->EndSourceFile() would destroy the ASTContext! // So just inform the preprocessor of EOF, while keeping everything alive. - Clang->getPreprocessor().EndSourceFile(); + PP.EndSourceFile(); // UnitDiagsConsumer is local, we can not store it in CompilerInstance that // has a longer lifetime. Clang->getDiagnostics().setClient(new IgnoreDiagnostics); diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 3b0af0ab50a62..08662697a4a5c 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -133,22 +133,19 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { CanonIncludes.addSystemHeadersMapping(CI.getLangOpts()); LangOpts = &CI.getLangOpts(); SourceMgr = &CI.getSourceManager(); + PP = &CI.getPreprocessor(); Includes.collect(CI); - if (Config::current().Diagnostics.UnusedIncludes == - Config::IncludesPolicy::Strict || - Config::current().Diagnostics.MissingIncludes == - Config::IncludesPolicy::Strict) - Pragmas.record(CI); + Pragmas.record(CI); if (BeforeExecuteCallback) BeforeExecuteCallback(CI); } std::unique_ptr createPPCallbacks() override { - assert(SourceMgr && LangOpts && - "SourceMgr and LangOpts must be set at this point"); + assert(SourceMgr && LangOpts && PP && + "SourceMgr, LangOpts and PP must be set at this point"); return std::make_unique( - std::make_unique(*SourceMgr, Macros), + std::make_unique(*PP, Macros), collectPragmaMarksCallback(*SourceMgr, Marks)); } @@ -215,6 +212,7 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { std::unique_ptr IWYUHandler = nullptr; const clang::LangOptions *LangOpts = nullptr; const SourceManager *SourceMgr = nullptr; + const Preprocessor *PP = nullptr; PreambleBuildStats *Stats; bool ParseForwardingFunctions; std::function BeforeExecuteCallback; @@ -382,7 +380,7 @@ scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) { PP.addPPCallbacks( std::make_unique(PP, SP.TextualDirectives)); PP.addPPCallbacks(collectPragmaMarksCallback(SM, SP.Marks)); - PP.addPPCallbacks(std::make_unique(SM, SP.Macros)); + PP.addPPCallbacks(std::make_unique(PP, SP.Macros)); if (llvm::Error Err = Action.Execute()) return std::move(Err); Action.EndSourceFile(); diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index 1b1220c8aa7a0..d4442c11a8ed0 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -1611,8 +1611,8 @@ declToHierarchyItem(const NamedDecl &ND, llvm::StringRef TUPath) { ASTContext &Ctx = ND.getASTContext(); auto &SM = Ctx.getSourceManager(); SourceLocation NameLoc = nameLocation(ND, Ctx.getSourceManager()); - SourceLocation BeginLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getBeginLoc())); - SourceLocation EndLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getEndLoc())); + SourceLocation BeginLoc = SM.getFileLoc(ND.getBeginLoc()); + SourceLocation EndLoc = SM.getFileLoc(ND.getEndLoc()); const auto DeclRange = toHalfOpenFileRange(SM, Ctx.getLangOpts(), {BeginLoc, EndLoc}); if (!DeclRange) diff --git a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp index 103e13f44d060..5ca45f94ea9f0 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp @@ -8,10 +8,25 @@ #include "AST.h" #include "Config.h" +#include "SourceCode.h" #include "refactor/Tweak.h" #include "support/Logger.h" #include "clang/AST/Decl.h" +#include "clang/AST/Expr.h" +#include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Type.h" +#include "clang/AST/TypeLoc.h" +#include "clang/Basic/LLVM.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Tooling/Core/Replacement.h" +#include "clang/Tooling/Syntax/Tokens.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/FormatVariadic.h" +#include "llvm/Support/raw_ostream.h" +#include +#include +#include namespace clang { namespace clangd { @@ -45,8 +60,12 @@ class AddUsing : public Tweak { // All of the following are set by prepare(). // The qualifier to remove. NestedNameSpecifierLoc QualifierToRemove; - // The name following QualifierToRemove. - llvm::StringRef Name; + // Qualified name to use when spelling the using declaration. This might be + // different than SpelledQualifier in presence of error correction. + std::string QualifierToSpell; + // The name and qualifier as spelled in the code. + llvm::StringRef SpelledQualifier; + llvm::StringRef SpelledName; // If valid, the insertion point for "using" statement must come after this. // This is relevant when the type is defined in the main file, to make sure // the type/function is already defined at the point where "using" is added. @@ -56,7 +75,7 @@ REGISTER_TWEAK(AddUsing) std::string AddUsing::title() const { return std::string(llvm::formatv( - "Add using-declaration for {0} and remove qualifier", Name)); + "Add using-declaration for {0} and remove qualifier", SpelledName)); } // Locates all "using" statements relevant to SelectionDeclContext. @@ -269,36 +288,23 @@ bool AddUsing::prepare(const Selection &Inputs) { if (Node == nullptr) return false; + SourceRange SpelledNameRange; if (auto *D = Node->ASTNode.get()) { - if (auto *II = D->getDecl()->getIdentifier()) { + if (D->getDecl()->getIdentifier()) { QualifierToRemove = D->getQualifierLoc(); - Name = II->getName(); + SpelledNameRange = D->getSourceRange(); MustInsertAfterLoc = D->getDecl()->getBeginLoc(); } } else if (auto *T = Node->ASTNode.get()) { if (auto E = T->getAs()) { QualifierToRemove = E.getQualifierLoc(); - if (!QualifierToRemove) - return false; - auto NameRange = E.getSourceRange(); + SpelledNameRange = E.getSourceRange(); if (auto T = E.getNamedTypeLoc().getAs()) { // Remove the template arguments from the name. - NameRange.setEnd(T.getLAngleLoc().getLocWithOffset(-1)); + SpelledNameRange.setEnd(T.getLAngleLoc().getLocWithOffset(-1)); } - auto SpelledTokens = TB.spelledForExpanded(TB.expandedTokens(NameRange)); - if (!SpelledTokens) - return false; - auto SpelledRange = syntax::Token::range(SM, SpelledTokens->front(), - SpelledTokens->back()); - Name = SpelledRange.text(SM); - - std::string QualifierToRemoveStr = getNNSLAsString( - QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy()); - if (!Name.consume_front(QualifierToRemoveStr)) - return false; // What's spelled doesn't match the qualifier. - if (const auto *ET = E.getTypePtr()) { if (const auto *TDT = dyn_cast(ET->getNamedType().getTypePtr())) { @@ -309,19 +315,14 @@ bool AddUsing::prepare(const Selection &Inputs) { } } } - - // FIXME: This only supports removing qualifiers that are made up of just - // namespace names. If qualifier contains a type, we could take the longest - // namespace prefix and remove that. - if (!QualifierToRemove.hasQualifier() || + if (!QualifierToRemove || + // FIXME: This only supports removing qualifiers that are made up of just + // namespace names. If qualifier contains a type, we could take the + // longest namespace prefix and remove that. !QualifierToRemove.getNestedNameSpecifier()->getAsNamespace() || - Name.empty()) { - return false; - } - - if (isNamespaceForbidden(Inputs, *QualifierToRemove.getNestedNameSpecifier())) + // Respect user config. + isNamespaceForbidden(Inputs, *QualifierToRemove.getNestedNameSpecifier())) return false; - // Macros are difficult. We only want to offer code action when what's spelled // under the cursor is a namespace qualifier. If it's a macro that expands to // a qualifier, user would not know what code action will actually change. @@ -333,23 +334,35 @@ bool AddUsing::prepare(const Selection &Inputs) { return false; } + auto SpelledTokens = + TB.spelledForExpanded(TB.expandedTokens(SpelledNameRange)); + if (!SpelledTokens) + return false; + auto SpelledRange = + syntax::Token::range(SM, SpelledTokens->front(), SpelledTokens->back()); + // We only drop qualifiers that're namespaces, so this is safe. + std::tie(SpelledQualifier, SpelledName) = + splitQualifiedName(SpelledRange.text(SM)); + QualifierToSpell = getNNSLAsString( + QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy()); + if (!llvm::StringRef(QualifierToSpell).endswith(SpelledQualifier) || + SpelledName.empty()) + return false; // What's spelled doesn't match the qualifier. return true; } Expected AddUsing::apply(const Selection &Inputs) { auto &SM = Inputs.AST->getSourceManager(); - std::string QualifierToRemoveStr = getNNSLAsString( - QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy()); tooling::Replacements R; if (auto Err = R.add(tooling::Replacement( SM, SM.getSpellingLoc(QualifierToRemove.getBeginLoc()), - QualifierToRemoveStr.length(), ""))) { + SpelledQualifier.size(), ""))) { return std::move(Err); } - auto InsertionPoint = - findInsertionPoint(Inputs, QualifierToRemove, Name, MustInsertAfterLoc); + auto InsertionPoint = findInsertionPoint(Inputs, QualifierToRemove, + SpelledName, MustInsertAfterLoc); if (!InsertionPoint) { return InsertionPoint.takeError(); } @@ -362,7 +375,7 @@ Expected AddUsing::apply(const Selection &Inputs) { if (InsertionPoint->AlwaysFullyQualify && !isFullyQualified(QualifierToRemove.getNestedNameSpecifier())) UsingTextStream << "::"; - UsingTextStream << QualifierToRemoveStr << Name << ";" + UsingTextStream << QualifierToSpell << SpelledName << ";" << InsertionPoint->Suffix; assert(SM.getFileID(InsertionPoint->Loc) == SM.getMainFileID()); diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt index 056e5803d711b..6d7bb59be3ab1 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -157,7 +157,6 @@ clang_target_link_libraries(ClangdTests clangLex clangSema clangSerialization - clangTesting clangTooling clangToolingCore clangToolingInclusions @@ -172,6 +171,7 @@ target_link_libraries(ClangdTests clangDaemon clangIncludeCleaner + clangTesting clangTidy clangdSupport ) diff --git a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp index 196ed5cea4693..163a7f1a31707 100644 --- a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp +++ b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp @@ -8,12 +8,14 @@ #include "AST.h" #include "Annotations.h" #include "CollectMacros.h" +#include "Matchers.h" #include "SourceCode.h" #include "TestTU.h" #include "clang/Basic/SourceLocation.h" #include "llvm/Support/ScopedPrinter.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include namespace clang { namespace clangd { @@ -21,19 +23,24 @@ namespace { using testing::UnorderedElementsAreArray; +MATCHER_P(rangeIs, R, "") { return arg.Rng == R; } +MATCHER(isDef, "") { return arg.IsDefinition; } +MATCHER(inConditionalDirective, "") { return arg.InConditionalDirective; } + TEST(CollectMainFileMacros, SelectedMacros) { // References of the same symbol must have the ranges with the same // name(integer). If there are N different symbols then they must be named // from 1 to N. Macros for which SymbolID cannot be computed must be named - // "Unknown". + // "Unknown". The payload of the annotation describes the extra bit + // information of the MacroOccurrence (e.g. $1(def) => IsDefinition). const char *Tests[] = { R"cpp(// Macros: Cursor on definition. - #define $1[[FOO]](x,y) (x + y) + #define $1(def)[[FOO]](x,y) (x + y) int main() { int x = $1[[FOO]]($1[[FOO]](3, 4), $1[[FOO]](5, 6)); } )cpp", R"cpp( - #define $1[[M]](X) X; - #define $2[[abc]] 123 + #define $1(def)[[M]](X) X; + #define $2(def)[[abc]] 123 int s = $1[[M]]($2[[abc]]); )cpp", // FIXME: Locating macro in duplicate definitions doesn't work. Enable @@ -48,31 +55,50 @@ TEST(CollectMainFileMacros, SelectedMacros) { // #undef $2[[abc]] // )cpp", R"cpp( - #ifdef $Unknown[[UNDEFINED]] + #ifdef $Unknown(condit)[[UNDEFINED]] + #endif + + #ifndef $Unknown(condit)[[UNDEFINED]] + #endif + + #if defined($Unknown(condit)[[UNDEFINED]]) #endif )cpp", R"cpp( - #ifndef $Unknown[[abc]] - #define $1[[abc]] - #ifdef $1[[abc]] + #ifndef $Unknown(condit)[[abc]] + #define $1(def)[[abc]] + #ifdef $1(condit)[[abc]] #endif #endif )cpp", R"cpp( // Macros from token concatenations not included. - #define $1[[CONCAT]](X) X##A() - #define $2[[PREPEND]](X) MACRO##X() - #define $3[[MACROA]]() 123 + #define $1(def)[[CONCAT]](X) X##A() + #define $2(def)[[PREPEND]](X) MACRO##X() + #define $3(def)[[MACROA]]() 123 int B = $1[[CONCAT]](MACRO); int D = $2[[PREPEND]](A); )cpp", R"cpp( - // FIXME: Macro names in a definition are not detected. - #define $1[[MACRO_ARGS2]](X, Y) X Y - #define $2[[FOO]] BAR - #define $3[[BAR]] 1 + #define $1(def)[[MACRO_ARGS2]](X, Y) X Y + #define $3(def)[[BAR]] 1 + #define $2(def)[[FOO]] $3[[BAR]] int A = $2[[FOO]]; )cpp"}; + auto ExpectedResults = [](const Annotations &T, StringRef Name) { + std::vector> ExpectedLocations; + for (const auto &[R, Bits] : T.rangesWithPayload(Name)) { + if (Bits == "def") + ExpectedLocations.push_back(testing::AllOf(rangeIs(R), isDef())); + else if (Bits == "condit") + ExpectedLocations.push_back( + testing::AllOf(rangeIs(R), inConditionalDirective())); + else + ExpectedLocations.push_back(testing::AllOf(rangeIs(R))); + } + return ExpectedLocations; + }; + for (const char *Test : Tests) { Annotations T(Test); auto AST = TestTU::withCode(T.code()).build(); @@ -80,13 +106,16 @@ TEST(CollectMainFileMacros, SelectedMacros) { auto &SM = AST.getSourceManager(); auto &PP = AST.getPreprocessor(); - // Known macros. - for (int I = 1;; I++) { - const auto ExpectedRefs = T.ranges(llvm::to_string(I)); - if (ExpectedRefs.empty()) - break; + for (const auto &[Name, Ranges] : T.all_ranges()) { + if (Name == "Unknown") { + EXPECT_THAT(ActualMacroRefs.UnknownMacros, + UnorderedElementsAreArray(ExpectedResults(T, "Unknown"))) + << "Unknown macros doesn't match in " << Test; + continue; + } - auto Loc = sourceLocationInMainFile(SM, ExpectedRefs.begin()->start); + auto Loc = sourceLocationInMainFile( + SM, offsetToPosition(T.code(), Ranges.front().Begin)); ASSERT_TRUE(bool(Loc)); const auto *Id = syntax::spelledIdentifierTouching(*Loc, AST.getTokens()); ASSERT_TRUE(Id); @@ -94,19 +123,11 @@ TEST(CollectMainFileMacros, SelectedMacros) { assert(Macro); auto SID = getSymbolID(Macro->Name, Macro->Info, SM); - std::vector Ranges; - for (const auto &Ref : ActualMacroRefs.MacroRefs[SID]) - Ranges.push_back(Ref.Rng); - EXPECT_THAT(ExpectedRefs, UnorderedElementsAreArray(Ranges)) - << "Annotation=" << I << ", MacroName=" << Macro->Name + EXPECT_THAT(ActualMacroRefs.MacroRefs[SID], + UnorderedElementsAreArray(ExpectedResults(T, Name))) + << "Annotation=" << Name << ", MacroName=" << Macro->Name << ", Test = " << Test; } - // Unknown macros. - std::vector Ranges; - for (const auto &Ref : AST.getMacros().UnknownMacros) - Ranges.push_back(Ref.Rng); - EXPECT_THAT(Ranges, UnorderedElementsAreArray(T.ranges("Unknown"))) - << "Unknown macros doesn't match in " << Test; } } } // namespace diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp index 211fd1311c98f..df46e2fa09951 100644 --- a/clang-tools-extra/clangd/unittests/HoverTests.cpp +++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp @@ -14,11 +14,12 @@ #include "TestTU.h" #include "index/MemIndex.h" #include "clang/AST/Attr.h" +#include "clang/Format/Format.h" #include "clang/Index/IndexSymbol.h" #include "llvm/ADT/StringRef.h" -#include "gmock/gmock.h" #include "gtest/gtest.h" +#include #include #include @@ -28,6 +29,10 @@ namespace { using PassMode = HoverInfo::PassType::PassMode; +std::string guard(llvm::StringRef Code) { + return "#pragma once\n" + Code.str(); +} + TEST(Hover, Structured) { struct { const char *const Code; @@ -2882,6 +2887,99 @@ TEST(Hover, All) { } } +TEST(Hover, Providers) { + struct { + const char *Code; + const std::function ExpectedBuilder; + } Cases[] = {{R"cpp( + struct Foo {}; + Foo F = Fo^o{}; + )cpp", + [](HoverInfo &HI) { HI.Provider = ""; }}, + {R"cpp( + #include "foo.h" + Foo F = Fo^o{}; + )cpp", + [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}, + {R"cpp( + #include "all.h" + Foo F = Fo^o{}; + )cpp", + [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}, + {R"cpp( + #define FOO 5 + int F = ^FOO; + )cpp", + [](HoverInfo &HI) { HI.Provider = ""; }}, + {R"cpp( + #include "foo.h" + int F = ^FOO; + )cpp", + [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}, + {R"cpp( + #include "all.h" + int F = ^FOO; + )cpp", + [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}, + {R"cpp( + #include "foo.h" + Foo A; + Foo B; + Foo C = A ^+ B; + )cpp", + [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}, + // Hover selects the underlying decl of the using decl + {R"cpp( + #include "foo.h" + namespace ns { + using ::Foo; + } + ns::F^oo d; + )cpp", + [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}}; + + for (const auto &Case : Cases) { + Annotations Code{Case.Code}; + SCOPED_TRACE(Code.code()); + + TestTU TU; + TU.Filename = "foo.cpp"; + TU.Code = Code.code(); + TU.AdditionalFiles["foo.h"] = guard(R"cpp( + #define FOO 1 + class Foo {}; + Foo& operator+(const Foo, const Foo); + )cpp"); + TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\""); + + auto AST = TU.build(); + auto H = getHover(AST, Code.point(), format::getLLVMStyle(), nullptr); + ASSERT_TRUE(H); + HoverInfo Expected; + Case.ExpectedBuilder(Expected); + SCOPED_TRACE(H->present().asMarkdown()); + EXPECT_EQ(H->Provider, Expected.Provider); + } +} + +TEST(Hover, ParseProviderInfo) { + HoverInfo HIFoo; + HIFoo.Name = "foo"; + HIFoo.Provider = "\"foo.h\""; + + HoverInfo HIFooBar; + HIFooBar.Name = "foo"; + HIFooBar.Provider = ""; + struct Case { + HoverInfo HI; + llvm::StringRef ExpectedMarkdown; + } Cases[] = {{HIFoo, "### `foo` \nprovided by `\"foo.h\"`"}, + {HIFooBar, "### `foo` \nprovided by ``"}}; + + for (const auto &Case : Cases) + EXPECT_EQ(Case.HI.present().asMarkdown(), Case.ExpectedMarkdown); +} + TEST(Hover, DocsFromIndex) { Annotations T(R"cpp( template class X {}; @@ -2947,6 +3045,15 @@ TEST(Hover, NoCrash) { getHover(AST, P, format::getLLVMStyle(), nullptr); } +TEST(Hover, NoCrashAPInt64) { + Annotations T(R"cpp( + constexpr unsigned long value = -1; // wrap around + void foo() { va^lue; } + )cpp"); + auto AST = TestTU::withCode(T.code()).build(); + getHover(AST, T.point(), format::getLLVMStyle(), nullptr); +} + TEST(Hover, DocsFromMostSpecial) { Annotations T(R"cpp( // doc1 @@ -3359,8 +3466,8 @@ TEST(Hover, ParseDocumentation) { } } -// This is a separate test as headings don't create any differences in plaintext -// mode. +// This is a separate test as headings don't create any differences in +// plaintext mode. TEST(Hover, PresentHeadings) { HoverInfo HI; HI.Kind = index::SymbolKind::Variable; diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp index 409e3cee791c3..69b4e07439c38 100644 --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -30,6 +30,7 @@ #include "gtest/gtest.h" #include #include +#include #include namespace clang { @@ -328,6 +329,26 @@ TEST(IncludeCleaner, NoDiagsForObjC) { ParsedAST AST = TU.build(); EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); } + +TEST(IncludeCleaner, UmbrellaUsesPrivate) { + TestTU TU; + TU.Code = R"cpp( + #include "private.h" + )cpp"; + TU.AdditionalFiles["private.h"] = guard(R"cpp( + // IWYU pragma: private, include "public.h" + void foo() {} + )cpp"); + TU.Filename = "public.h"; + Config Cfg; + Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict; + WithContextValue Ctx(Config::Key, std::move(Cfg)); + ParsedAST AST = TU.build(); + EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); + IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); + EXPECT_THAT(Findings.UnusedIncludes, IsEmpty()); +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp index 259efcf54a6b2..975378118b7ad 100644 --- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -399,7 +399,7 @@ TEST(SemanticHighlighting, GetsCorrectTokens) { #define $Macro_decl[[MACRO_CONCAT]](X, V, T) T foo##X = V #define $Macro_decl[[DEF_VAR]](X, V) int X = V #define $Macro_decl[[DEF_VAR_T]](T, X, V) T X = V - #define $Macro_decl[[DEF_VAR_REV]](V, X) DEF_VAR(X, V) + #define $Macro_decl[[DEF_VAR_REV]](V, X) $Macro[[DEF_VAR]](X, V) #define $Macro_decl[[CPY]](X) X #define $Macro_decl[[DEF_VAR_TYPE]](X, Y) X Y #define $Macro_decl[[SOME_NAME]] variable @@ -431,7 +431,7 @@ TEST(SemanticHighlighting, GetsCorrectTokens) { )cpp", R"cpp( #define $Macro_decl[[fail]](expr) expr - #define $Macro_decl[[assert]](COND) if (!(COND)) { fail("assertion failed" #COND); } + #define $Macro_decl[[assert]](COND) if (!(COND)) { $Macro[[fail]]("assertion failed" #COND); } // Preamble ends. int $Variable_def[[x]]; int $Variable_def[[y]]; diff --git a/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp index adfd018f56d27..f3a479a9a240f 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp @@ -8,8 +8,12 @@ #include "Config.h" #include "TweakTesting.h" -#include "gmock/gmock.h" +#include "support/Context.h" +#include "llvm/ADT/StringMap.h" +#include "llvm/ADT/StringRef.h" #include "gtest/gtest.h" +#include +#include namespace clang { namespace clangd { @@ -30,7 +34,7 @@ namespace one { void oo() {} template class tt {}; namespace two { -enum ee {}; +enum ee { ee_enum_value }; void ff() {} class cc { public: @@ -64,9 +68,6 @@ class cc { EXPECT_UNAVAILABLE(Header + "void fun() { ::ban::fo^o(); }"); EXPECT_AVAILABLE(Header + "void fun() { banana::fo^o(); }"); - // Do not offer code action on typo-corrections. - EXPECT_UNAVAILABLE(Header + "/*error-ok*/c^c C;"); - // NestedNameSpecifier, but no namespace. EXPECT_UNAVAILABLE(Header + "class Foo {}; class F^oo foo;"); @@ -96,16 +97,17 @@ TEST_F(AddUsingTest, Apply) { struct { llvm::StringRef TestSource; llvm::StringRef ExpectedSource; - } Cases[]{{ - // Function, no other using, namespace. - R"cpp( + } Cases[]{ + { + // Function, no other using, namespace. + R"cpp( #include "test.hpp" namespace { void fun() { ^one::two::ff(); } })cpp", - R"cpp( + R"cpp( #include "test.hpp" namespace {using one::two::ff; @@ -113,17 +115,17 @@ void fun() { ff(); } })cpp", - }, - // Type, no other using, namespace. - { - R"cpp( + }, + // Type, no other using, namespace. + { + R"cpp( #include "test.hpp" namespace { void fun() { ::one::t^wo::cc inst; } })cpp", - R"cpp( + R"cpp( #include "test.hpp" namespace {using ::one::two::cc; @@ -131,16 +133,16 @@ void fun() { cc inst; } })cpp", - }, - // Type, no other using, no namespace. - { - R"cpp( + }, + // Type, no other using, no namespace. + { + R"cpp( #include "test.hpp" void fun() { one::two::e^e inst; })cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::two::ee; @@ -148,9 +150,9 @@ using one::two::ee; void fun() { ee inst; })cpp"}, - // Function, other usings. - { - R"cpp( + // Function, other usings. + { + R"cpp( #include "test.hpp" using one::two::cc; @@ -161,7 +163,7 @@ void fun() { one::two::f^f(); } })cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::two::cc; @@ -172,10 +174,10 @@ void fun() { ff(); } })cpp", - }, - // Function, other usings inside namespace. - { - R"cpp( + }, + // Function, other usings inside namespace. + { + R"cpp( #include "test.hpp" using one::two::cc; @@ -188,7 +190,7 @@ void fun() { o^ne::oo(); } })cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::two::cc; @@ -201,9 +203,9 @@ void fun() { oo(); } })cpp"}, - // Using comes after cursor. - { - R"cpp( + // Using comes after cursor. + { + R"cpp( #include "test.hpp" namespace { @@ -215,7 +217,7 @@ void fun() { using one::two::cc; })cpp", - R"cpp( + R"cpp( #include "test.hpp" namespace {using one::two::ff; @@ -228,14 +230,14 @@ void fun() { using one::two::cc; })cpp"}, - // Pointer type. - {R"cpp( + // Pointer type. + {R"cpp( #include "test.hpp" void fun() { one::two::c^c *p; })cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::two::cc; @@ -243,8 +245,8 @@ using one::two::cc; void fun() { cc *p; })cpp"}, - // Namespace declared via macro. - {R"cpp( + // Namespace declared via macro. + {R"cpp( #include "test.hpp" #define NS_BEGIN(name) namespace name { @@ -254,7 +256,7 @@ void fun() { one::two::f^f(); } })cpp", - R"cpp( + R"cpp( #include "test.hpp" #define NS_BEGIN(name) namespace name { @@ -266,15 +268,15 @@ void fun() { ff(); } })cpp"}, - // Inside macro argument. - {R"cpp( + // Inside macro argument. + {R"cpp( #include "test.hpp" #define CALL(name) name() void fun() { CALL(one::t^wo::ff); })cpp", - R"cpp( + R"cpp( #include "test.hpp" #define CALL(name) name() @@ -283,15 +285,15 @@ using one::two::ff; void fun() { CALL(ff); })cpp"}, - // Parent namespace != lexical parent namespace - {R"cpp( + // Parent namespace != lexical parent namespace + {R"cpp( #include "test.hpp" namespace foo { void fun(); } void foo::fun() { one::two::f^f(); })cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::two::ff; @@ -300,8 +302,8 @@ namespace foo { void fun(); } void foo::fun() { ff(); })cpp"}, - // If all other using are fully qualified, add :: - {R"cpp( + // If all other using are fully qualified, add :: + {R"cpp( #include "test.hpp" using ::one::two::cc; @@ -310,7 +312,7 @@ using ::one::two::ee; void fun() { one::two::f^f(); })cpp", - R"cpp( + R"cpp( #include "test.hpp" using ::one::two::cc; @@ -319,8 +321,8 @@ using ::one::two::ff;using ::one::two::ee; void fun() { ff(); })cpp"}, - // Make sure we don't add :: if it's already there - {R"cpp( + // Make sure we don't add :: if it's already there + {R"cpp( #include "test.hpp" using ::one::two::cc; @@ -329,7 +331,7 @@ using ::one::two::ee; void fun() { ::one::two::f^f(); })cpp", - R"cpp( + R"cpp( #include "test.hpp" using ::one::two::cc; @@ -338,8 +340,8 @@ using ::one::two::ff;using ::one::two::ee; void fun() { ff(); })cpp"}, - // If even one using doesn't start with ::, do not add it - {R"cpp( + // If even one using doesn't start with ::, do not add it + {R"cpp( #include "test.hpp" using ::one::two::cc; @@ -348,7 +350,7 @@ using one::two::ee; void fun() { one::two::f^f(); })cpp", - R"cpp( + R"cpp( #include "test.hpp" using ::one::two::cc; @@ -357,14 +359,14 @@ using one::two::ff;using one::two::ee; void fun() { ff(); })cpp"}, - // using alias; insert using for the spelled name. - {R"cpp( + // using alias; insert using for the spelled name. + {R"cpp( #include "test.hpp" void fun() { one::u^u u; })cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::uu; @@ -372,29 +374,29 @@ using one::uu; void fun() { uu u; })cpp"}, - // using namespace. - {R"cpp( + // using namespace. + {R"cpp( #include "test.hpp" using namespace one; namespace { two::c^c C; })cpp", - R"cpp( + R"cpp( #include "test.hpp" using namespace one; namespace {using two::cc; cc C; })cpp"}, - // Type defined in main file, make sure using is after that. - {R"cpp( + // Type defined in main file, make sure using is after that. + {R"cpp( namespace xx { struct yy {}; } x^x::yy X; )cpp", - R"cpp( + R"cpp( namespace xx { struct yy {}; } @@ -403,8 +405,8 @@ using xx::yy; yy X; )cpp"}, - // Type defined in main file via "using", insert after that. - {R"cpp( + // Type defined in main file via "using", insert after that. + {R"cpp( #include "test.hpp" namespace xx { @@ -413,7 +415,7 @@ namespace xx { x^x::yy X; )cpp", - R"cpp( + R"cpp( #include "test.hpp" namespace xx { @@ -424,8 +426,8 @@ using xx::yy; yy X; )cpp"}, - // Using must come after function definition. - {R"cpp( + // Using must come after function definition. + {R"cpp( namespace xx { void yy(); } @@ -434,7 +436,7 @@ void fun() { x^x::yy(); } )cpp", - R"cpp( + R"cpp( namespace xx { void yy(); } @@ -445,28 +447,58 @@ void fun() { yy(); } )cpp"}, - // Existing using with non-namespace part. - {R"cpp( + // Existing using with non-namespace part. + {R"cpp( #include "test.hpp" using one::two::ee::ee_one; one::t^wo::cc c; )cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::two::cc;using one::two::ee::ee_one; cc c; )cpp"}, - // Template (like std::vector). - {R"cpp( + // Template (like std::vector). + {R"cpp( #include "test.hpp" one::v^ec foo; )cpp", - R"cpp( + R"cpp( #include "test.hpp" using one::vec; vec foo; -)cpp"}}; +)cpp"}, + // Typo correction. + {R"cpp( +// error-ok +#include "test.hpp" +c^c C; +)cpp", + R"cpp( +// error-ok +#include "test.hpp" +using one::two::cc; + +cc C; +)cpp"}, + {R"cpp( +// error-ok +#include "test.hpp" +void foo() { + switch(one::two::ee{}) { case two::ee_^one:break; } +} +)cpp", + R"cpp( +// error-ok +#include "test.hpp" +using one::two::ee_one; + +void foo() { + switch(one::two::ee{}) { case ee_one:break; } +} +)cpp"}, + }; llvm::StringMap EditedFiles; for (const auto &Case : Cases) { ExtraFiles["test.hpp"] = R"cpp( @@ -484,6 +516,8 @@ class cc { using uu = two::cc; template struct vec {}; })cpp"; + // Typo correction is disabled in msvc-compatibility mode. + ExtraArgs.push_back("-fno-ms-compatibility"); EXPECT_EQ(apply(Case.TestSource, &EditedFiles), Case.ExpectedSource); } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 3f79e8e2a187a..03ba9e160b738 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -133,6 +133,13 @@ New checks Checks that all implicit and explicit inline functions in header files are tagged with the ``LIBC_INLINE`` macro. +- New :doc:`readability-avoid-unconditional-preprocessor-if + ` check. + + Finds code blocks that are constantly enabled or disabled in preprocessor + directives by analyzing ``#if`` conditions, such as ``#if 0`` and + ``#if 1``, etc. + New check aliases ^^^^^^^^^^^^^^^^^ @@ -162,6 +169,10 @@ Changes in existing checks ` check. Global options of the same name should be used instead. +- Improved :doc:`bugprone-use-after-move + ` check to also cover constructor + initializers. + - Deprecated check-local options `HeaderFileExtensions` in :doc:`google-build-namespaces ` check. @@ -192,6 +203,11 @@ Changes in existing checks constructors toward hand written constructors so that they are skipped if more than one exists. +- Fixed false positive in :doc:`modernize-use-equals-default + ` check for special member + functions containing macros or preprocessor directives, and out-of-line special + member functions in unions. + - Fixed reading `HungarianNotation.CString.*` options in :doc:`readability-identifier-naming ` check. @@ -207,6 +223,9 @@ Changes in existing checks behavior of using `i` as the prefix for enum tags, set the `EnumConstantPrefix` option to `i` instead of using `EnumConstantHungarianPrefix`. +- Added support to optionally ignore user-defined literals in + :doc:`readability-magic-numbers`. + - Fixed a false positive in :doc:`readability-container-size-empty ` check when comparing ``std::array`` objects to default constructed ones. The behavior for this and @@ -234,6 +253,10 @@ Changes in existing checks string for ``Prefix`` or ``Suffix`` options could result in the style not being used. +- Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name + ` when using + ``DISABLED_`` in the test suite name. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name.rst b/clang-tools-extra/docs/clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name.rst index f2053b4d2fcd3..e667fd12222bb 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name.rst @@ -3,8 +3,8 @@ google-readability-avoid-underscore-in-googletest-name ====================================================== -Checks whether there are underscores in googletest test and test case names in -test macros: +Checks whether there are underscores in googletest test suite names and test +names in test macros: - ``TEST`` - ``TEST_F`` @@ -18,17 +18,17 @@ For example: .. code-block:: c++ - TEST(TestCaseName, Illegal_TestName) {} - TEST(Illegal_TestCaseName, TestName) {} + TEST(TestSuiteName, Illegal_TestName) {} + TEST(Illegal_TestSuiteName, TestName) {} -would trigger the check. `Underscores are not allowed`_ in test names nor test -case names. +would trigger the check. `Underscores are not allowed`_ in test suite name nor +test names. -The ``DISABLED_`` prefix, which may be used to `disable individual tests`_, is -ignored when checking test names, but the rest of the rest of the test name is -still checked. +The ``DISABLED_`` prefix, which may be used to +`disable test suites and individual tests`_, is removed from the test suite name +and test name before checking for underscores. This check does not propose any fixes. .. _Underscores are not allowed: https://google.github.io/googletest/faq.html#why-should-test-suite-names-and-test-names-not-contain-underscore -.. _disable individual tests: https://google.github.io/googletest/advanced.html#temporarily-disabling-tests +.. _disable test suites and individual tests: https://google.github.io/googletest/advanced.html#temporarily-disabling-tests diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index f1945c1b5bf05..095f32f38da77 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -331,6 +331,7 @@ Clang-Tidy Checks `portability-simd-intrinsics `_, `portability-std-allocator-const `_, `readability-avoid-const-params-in-decls `_, "Yes" + `readability-avoid-unconditional-preprocessor-if `_, `readability-braces-around-statements `_, "Yes" `readability-const-return-type `_, "Yes" `readability-container-contains `_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst index 5509a5414f8c5..7bd6fa93ba1ea 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-equals-default.rst @@ -32,5 +32,6 @@ Options .. option:: IgnoreMacros - If set to `true`, the check will not give warnings inside macros. Default - is `true`. + If set to `true`, the check will not give warnings inside macros and will + ignore special members with bodies contain macros or preprocessor directives. + Default is `true`. diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst new file mode 100644 index 0000000000000..ce3bfaffac380 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst @@ -0,0 +1,32 @@ +.. title:: clang-tidy - readability-avoid-unconditional-preprocessor-if + +readability-avoid-unconditional-preprocessor-if +=============================================== + +Finds code blocks that are constantly enabled or disabled in preprocessor +directives by analyzing ``#if`` conditions, such as ``#if 0`` and ``#if 1``, +etc. + +.. code-block:: c++ + + #if 0 + // some disabled code + #endif + + #if 1 + // some enabled code that can be disabled manually + #endif + +Unconditional preprocessor directives, such as ``#if 0`` for disabled code +and ``#if 1`` for enabled code, can lead to dead code and always enabled code, +respectively. Dead code can make understanding the codebase more difficult, +hinder readability, and may be a sign of unfinished functionality or abandoned +features. This can cause maintenance issues, confusion for future developers, +and potential compilation problems. + +As a solution for both cases, consider using preprocessor macros or defines, +like ``#ifdef DEBUGGING_ENABLED``, to control code enabling or disabling. +This approach provides better coordination and flexibility when working with +different parts of the codebase. Alternatively, you can comment out the entire +code using ``/* */`` block comments and add a hint, such as ``@todo``, +to indicate future actions. diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/magic-numbers.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/magic-numbers.rst index b5017f81d0a59..4b3d7190d4345 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/magic-numbers.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/magic-numbers.rst @@ -57,7 +57,7 @@ Example with magic values refactored: const size_t NUMBER_OF_ELEMENTS = 30; using containerType = CustomType; - + struct OtherType { containerType container; } @@ -144,3 +144,8 @@ Options Boolean value indicating whether to accept magic numbers in ``typedef`` or ``using`` declarations. Default value is `false`. + +.. option:: IgnoreUserDefinedLiterals + + Boolean value indicating whether to accept magic numbers in user-defined + literals. Default value is `false`. diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h index cd11700548075..66916a52046cb 100644 --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h +++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h @@ -16,11 +16,13 @@ #include "clang/Format/Format.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/STLFunctionalExtras.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/Support/MemoryBufferRef.h" #include namespace clang { class SourceLocation; +class SourceManager; class Decl; class FileEntry; class HeaderSearch; @@ -75,6 +77,14 @@ std::string fixIncludes(const AnalysisResults &Results, llvm::StringRef Code, std::string spellHeader(const Header &H, HeaderSearch &HS, const FileEntry *Main); + +/// Gets all the providers for a symbol by traversing each location. +/// Returned headers are sorted by relevance, first element is the most +/// likely provider for the symbol. +llvm::SmallVector
headersForSymbol(const Symbol &S, + const SourceManager &SM, + const PragmaIncludes *PI); + } // namespace include_cleaner } // namespace clang diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index 6237bdb46babf..58402cce0b717 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -10,7 +10,6 @@ #include "AnalysisInternal.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" -#include "clang/AST/ASTContext.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclBase.h" #include "clang/Basic/SourceManager.h" @@ -19,9 +18,13 @@ #include "clang/Tooling/Core/Replacement.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" +#include "llvm/ADT/StringSet.h" +#include "llvm/Support/Error.h" +#include namespace clang::include_cleaner { @@ -90,9 +93,25 @@ AnalysisResults analyze(llvm::ArrayRef ASTRoots, }); AnalysisResults Results; - for (const Include &I : Inc.all()) - if (!Used.contains(&I) && PI && !PI->shouldKeep(I.Line)) - Results.Unused.push_back(&I); + for (const Include &I : Inc.all()) { + if (Used.contains(&I) || !I.Resolved) + continue; + if (PI) { + if (PI->shouldKeep(I.Line)) + continue; + // Check if main file is the public interface for a private header. If so + // we shouldn't diagnose it as unused. + if (auto PHeader = PI->getPublic(I.Resolved); !PHeader.empty()) { + PHeader = PHeader.trim("<>\""); + // Since most private -> public mappings happen in a verbatim way, we + // check textually here. This might go wrong in presence of symlinks or + // header mappings. But that's not different than rest of the places. + if (MainFile->tryGetRealPathName().endswith(PHeader)) + continue; + } + } + Results.Unused.push_back(&I); + } for (llvm::StringRef S : Missing.keys()) Results.Missing.push_back(S.str()); llvm::sort(Results.Missing); diff --git a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h index acf462919344b..6bfed91b584b3 100644 --- a/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h +++ b/clang-tools-extra/include-cleaner/lib/AnalysisInternal.h @@ -22,6 +22,7 @@ #define CLANG_INCLUDE_CLEANER_ANALYSISINTERNAL_H #include "TypesInternal.h" +#include "clang-include-cleaner/Analysis.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" #include "llvm/ADT/STLFunctionalExtras.h" @@ -58,13 +59,6 @@ llvm::SmallVector> findHeaders(const SymbolLocation &Loc, /// A set of locations that provides the declaration. std::vector> locateSymbol(const Symbol &S); -/// Gets all the providers for a symbol by traversing each location. -/// Returned headers are sorted by relevance, first element is the most -/// likely provider for the symbol. -llvm::SmallVector
headersForSymbol(const Symbol &S, - const SourceManager &SM, - const PragmaIncludes *PI); - /// Write an HTML summary of the analysis to the given stream. void writeHTMLReport(FileID File, const Includes &, llvm::ArrayRef Roots, diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp index 0ca84145721a6..e70a24367d6a9 100644 --- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp +++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp @@ -7,16 +7,19 @@ //===----------------------------------------------------------------------===// #include "AnalysisInternal.h" +#include "clang-include-cleaner/Types.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/TemplateBase.h" #include "clang/AST/TemplateName.h" #include "clang/AST/Type.h" #include "clang/AST/TypeLoc.h" #include "clang/Basic/SourceLocation.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/Support/Casting.h" namespace clang::include_cleaner { @@ -62,6 +65,24 @@ class ASTWalker : public RecursiveASTVisitor { return resolveTemplateName(TST->getTemplateName()); return Base->getAsRecordDecl(); } + // Templated as TemplateSpecializationType and + // DeducedTemplateSpecializationType doesn't share a common base. + template + // Picks the most specific specialization for a + // (Deduced)TemplateSpecializationType, while prioritizing using-decls. + NamedDecl *getMostRelevantTemplatePattern(const T *TST) { + // This is the underlying decl used by TemplateSpecializationType, can be + // null when type is dependent. + auto *RD = TST->getAsTagDecl(); + auto *ND = resolveTemplateName(TST->getTemplateName()); + // In case of exported template names always prefer the using-decl. This + // implies we'll point at the using-decl even when there's an explicit + // specializaiton using the exported name, but that's rare. + if (llvm::isa_and_present(ND)) + return ND; + // Fallback to primary template for dependent instantiations. + return RD ? RD : ND; + } public: ASTWalker(DeclCallback Callback) : Callback(Callback) {} @@ -161,17 +182,15 @@ class ASTWalker : public RecursiveASTVisitor { } bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc TL) { - // FIXME: Handle explicit specializations. report(TL.getTemplateNameLoc(), - resolveTemplateName(TL.getTypePtr()->getTemplateName())); + getMostRelevantTemplatePattern(TL.getTypePtr())); return true; } bool VisitDeducedTemplateSpecializationTypeLoc( DeducedTemplateSpecializationTypeLoc TL) { - // FIXME: Handle specializations. report(TL.getTemplateNameLoc(), - resolveTemplateName(TL.getTypePtr()->getTemplateName())); + getMostRelevantTemplatePattern(TL.getTypePtr())); return true; } diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp index c34c6c0a29a81..fc7d14582f632 100644 --- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -8,22 +8,29 @@ #include "clang-include-cleaner/Analysis.h" #include "AnalysisInternal.h" +#include "TypesInternal.h" #include "clang-include-cleaner/Record.h" #include "clang-include-cleaner/Types.h" #include "clang/AST/ASTContext.h" #include "clang/Basic/FileManager.h" +#include "clang/Basic/IdentifierTable.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Testing/TestAST.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/ScopedPrinter.h" #include "llvm/Testing/Annotations/Annotations.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include +#include +#include +#include namespace clang::include_cleaner { namespace { @@ -177,8 +184,31 @@ TEST_F(WalkUsedTest, MacroRefs) { Pair(Code.point("2"), UnorderedElementsAre(HdrFile)))); } -TEST(Analyze, Basic) { +class AnalyzeTest : public testing::Test { +protected: TestInputs Inputs; + PragmaIncludes PI; + RecordedPP PP; + AnalyzeTest() { + Inputs.MakeAction = [this] { + struct Hook : public SyntaxOnlyAction { + public: + Hook(RecordedPP &PP, PragmaIncludes &PI) : PP(PP), PI(PI) {} + bool BeginSourceFileAction(clang::CompilerInstance &CI) override { + CI.getPreprocessor().addPPCallbacks(PP.record(CI.getPreprocessor())); + PI.record(CI); + return true; + } + + RecordedPP &PP; + PragmaIncludes &PI; + }; + return std::make_unique(PP, PI); + }; + } +}; + +TEST_F(AnalyzeTest, Basic) { Inputs.Code = R"cpp( #include "a.h" #include "b.h" @@ -193,25 +223,6 @@ int x = a + c; )cpp"); Inputs.ExtraFiles["c.h"] = guard("int c;"); Inputs.ExtraFiles["keep.h"] = guard(""); - - RecordedPP PP; - PragmaIncludes PI; - Inputs.MakeAction = [&PP, &PI] { - struct Hook : public SyntaxOnlyAction { - public: - Hook(RecordedPP &PP, PragmaIncludes &PI) : PP(PP), PI(PI) {} - bool BeginSourceFileAction(clang::CompilerInstance &CI) override { - CI.getPreprocessor().addPPCallbacks(PP.record(CI.getPreprocessor())); - PI.record(CI); - return true; - } - - RecordedPP &PP; - PragmaIncludes &PI; - }; - return std::make_unique(PP, PI); - }; - TestAST AST(Inputs); auto Decls = AST.context().getTranslationUnitDecl()->decls(); auto Results = @@ -225,6 +236,30 @@ int x = a + c; EXPECT_THAT(Results.Unused, ElementsAre(B)); } +TEST_F(AnalyzeTest, PrivateUsedInPublic) { + // Check that umbrella header uses private include. + Inputs.Code = R"cpp(#include "private.h")cpp"; + Inputs.ExtraFiles["private.h"] = + guard("// IWYU pragma: private, include \"public.h\""); + Inputs.FileName = "public.h"; + TestAST AST(Inputs); + EXPECT_FALSE(PP.Includes.all().empty()); + auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(), + AST.preprocessor().getHeaderSearchInfo()); + EXPECT_THAT(Results.Unused, testing::IsEmpty()); +} + +TEST_F(AnalyzeTest, NoCrashWhenUnresolved) { + // Check that umbrella header uses private include. + Inputs.Code = R"cpp(#include "not_found.h")cpp"; + Inputs.ErrorOK = true; + TestAST AST(Inputs); + EXPECT_FALSE(PP.Includes.all().empty()); + auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(), + AST.preprocessor().getHeaderSearchInfo()); + EXPECT_THAT(Results.Unused, testing::IsEmpty()); +} + TEST(FixIncludes, Basic) { llvm::StringRef Code = R"cpp( #include "a.h" diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp index 68b6b217a2e01..8fcc2b5886ae4 100644 --- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp @@ -114,6 +114,25 @@ TEST(WalkAST, TagType) { // One explicit call from the TypeLoc in constructor spelling, another // implicit reference through the constructor call. testWalk("struct $explicit^$implicit^S { static int x; };", "auto y = ^S();"); + testWalk("template struct $explicit^Foo {};", "^Foo x;"); + testWalk(R"cpp( + template struct Foo {}; + template<> struct $explicit^Foo {};)cpp", + "^Foo x;"); + testWalk(R"cpp( + template struct Foo {}; + template struct $explicit^Foo { void x(); };)cpp", + "^Foo x;"); + testWalk(R"cpp( + template struct Foo {}; + template struct $explicit^Foo;)cpp", + "^Foo x;"); + // FIXME: This is broken due to + // https://github.com/llvm/llvm-project/issues/42259. + testWalk(R"cpp( + template struct $explicit^Foo { Foo(T); }; + template<> struct Foo { void get(); Foo(int); };)cpp", + "^Foo x(3);"); } TEST(WalkAST, Alias) { @@ -124,6 +143,25 @@ TEST(WalkAST, Alias) { "int y = ^x;"); testWalk("using $explicit^foo = int;", "^foo x;"); testWalk("struct S {}; using $explicit^foo = S;", "^foo x;"); + testWalk(R"cpp( + template struct Foo {}; + template<> struct Foo {}; + namespace ns { using ::$explicit^Foo; })cpp", + "ns::^Foo x;"); + testWalk(R"cpp( + template struct Foo {}; + namespace ns { using ::Foo; } + template<> struct ns::$explicit^Foo {};)cpp", + "^Foo x;"); + // AST doesn't have enough information to figure out whether specialization + // happened through an exported type or not. So err towards attributing use to + // the using-decl, specializations on the exported type should be rare and + // they're not permitted on type-aliases. + testWalk(R"cpp( + template struct Foo {}; + namespace ns { using ::$explicit^Foo; } + template<> struct ns::Foo {};)cpp", + "ns::^Foo x;"); } TEST(WalkAST, Using) { @@ -183,10 +221,6 @@ TEST(WalkAST, TemplateNames) { template