hokein created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
hokein requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
By extending and using the existing MainFileMacros structure.
- record more information (InConditionalDirective) in MacroOccurrence
- collect macro references inside macro body
When emiting include-cleaner diagnostics, we convert the result to the
RecordedPP structure just-in-time to interop with include-cleaner
Put all together into a patch (might need to split)
TODO: the newly-added test doesn't pass, because there are some other
issues in missing-include diagnostics.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D146017
Files:
clang-tools-extra/clangd/CollectMacros.cpp
clang-tools-extra/clangd/CollectMacros.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -399,7 +399,7 @@
#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 @@
)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]];
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -25,6 +25,7 @@
#include "llvm/Support/Casting.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Testing/Annotations/Annotations.h"
#include "llvm/Testing/Support/SupportHelpers.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -51,7 +52,12 @@
"Diag at " + llvm::to_string(Range) + " = [" + Message + "]") {
return arg.Range == Range && arg.Message == Message;
}
-
+MATCHER_P(Range, R, "") {
+ return arg.SymRefRange == R;
+}
+MATCHER_P(SingleHeader, Header, "") {
+ return arg.Providers.size() == 1 && arg.Providers[0] == Header;
+}
MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
::testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -169,6 +175,53 @@
EXPECT_THAT(Findings.MissingIncludes, ElementsAre(BInfo));
}
+TEST(IncludeCleaner, MissingIncludesForMacroRefs) {
+ Config Cfg;
+ Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict;
+ WithContextValue Ctx(Config::Key, std::move(Cfg));
+ llvm::StringRef TestCases[] = {
+ R"cpp(
+ #include "all.h"
+ int a = [[FOO]];
+ )cpp",
+
+ R"cpp(
+ #include "all.h"
+ #define BAR [[FOO]];
+ )cpp",
+
+ // No missing include insertion for ambiguous macro refs.
+ R"cpp(
+ #include "all.h"
+ #if defined(FOO)
+ #endif
+
+ #ifdef FOO
+ #endif
+ )cpp",
+ };
+
+ for (const auto& T : TestCases) {
+ TestTU TU;
+ llvm::Annotations MainFile = T;
+ TU.Code = MainFile.code();
+ TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\"");
+ TU.AdditionalFiles["foo.h"] = guard("#define FOO 1");
+ ParsedAST AST = TU.build();
+ const auto &SM = AST.getSourceManager();
+ include_cleaner::Header FooH = *SM.getFileManager().getFile("foo.h");
+
+ std::vector<testing::Matcher<MissingIncludeDiagInfo>> Matcher;
+ for (const auto &R : MainFile.ranges())
+ Matcher.push_back(
+ AllOf(Range(syntax::FileRange(AST.getSourceManager().getMainFileID(),
+ R.Begin, R.End)),
+ SingleHeader(FooH)));
+ EXPECT_THAT(computeIncludeCleanerFindings(AST).MissingIncludes,
+ testing::UnorderedElementsAreArray(Matcher));
+ }
+}
+
TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
Config Cfg;
Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict;
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -126,6 +126,7 @@
CanonIncludes.addSystemHeadersMapping(CI.getLangOpts());
LangOpts = &CI.getLangOpts();
SourceMgr = &CI.getSourceManager();
+ PP = &CI.getPreprocessor();
Includes.collect(CI);
if (Config::current().Diagnostics.UnusedIncludes ==
Config::IncludesPolicy::Strict ||
@@ -137,11 +138,11 @@
}
std::unique_ptr<PPCallbacks> 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<PPChainedCallbacks>(
- std::make_unique<CollectMainFileMacros>(*SourceMgr, Macros),
+ std::make_unique<CollectMainFileMacros>(*SourceMgr, *PP, Macros),
collectPragmaMarksCallback(*SourceMgr, Marks));
}
@@ -208,6 +209,7 @@
std::unique_ptr<CommentHandler> IWYUHandler = nullptr;
const clang::LangOptions *LangOpts = nullptr;
const SourceManager *SourceMgr = nullptr;
+ const Preprocessor *PP = nullptr;
PreambleBuildStats *Stats;
bool ParseForwardingFunctions;
std::function<void(CompilerInstance &)> BeforeExecuteCallback;
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -608,8 +608,8 @@
if (Preamble)
Macros = Preamble->Macros;
Clang->getPreprocessor().addPPCallbacks(
- std::make_unique<CollectMainFileMacros>(Clang->getSourceManager(),
- Macros));
+ std::make_unique<CollectMainFileMacros>(
+ Clang->getSourceManager(), Clang->getPreprocessor(), Macros));
std::vector<PragmaMark> Marks;
// FIXME: We need to patch the marks for stale preambles.
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -164,22 +164,34 @@
}
std::vector<include_cleaner::SymbolReference>
-collectMacroReferences(ParsedAST &AST) {
+convertMacroReferences(ParsedAST &AST) {
const auto &SM = AST.getSourceManager();
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector<include_cleaner::SymbolReference> Macros;
+ const auto &MacroRefs = AST.getMacros();
auto &PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
- AST.getTokens().spelledTokens(SM.getMainFileID())) {
- auto Macro = locateMacroAt(Tok, PP);
- if (!Macro)
- continue;
- if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid())
- Macros.push_back(
- {Tok.location(),
- include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
- DefLoc},
- include_cleaner::RefType::Explicit});
+ std::vector<include_cleaner::SymbolReference> Macros;
+ for (const auto &MAndRefs : MacroRefs.MacroRefs) {
+ for (const auto &Ref : MAndRefs.second) {
+ auto L = sourceLocationInMainFile(SM, Ref.Rng.start);
+ if (!L) {
+ llvm::consumeError(L.takeError());
+ continue;
+ }
+ if (const auto *Tok =
+ syntax::spelledIdentifierTouching(*L, AST.getTokens())) {
+ auto Macro = locateMacroAt(*Tok, PP);
+ if (!Macro)
+ continue;
+
+ if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid())
+ Macros.push_back(
+ {Tok->location(),
+ include_cleaner::Macro{
+ /*Name=*/PP.getIdentifierInfo(Tok->text(SM)), DefLoc},
+ Ref.InConditionalDirective
+ ? include_cleaner::RefType::Implicit
+ : include_cleaner::RefType::Explicit});
+ }
+ }
}
return Macros;
}
@@ -350,7 +362,7 @@
const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
std::vector<include_cleaner::SymbolReference> Macros =
- collectMacroReferences(AST);
+ convertMacroReferences(AST);
std::vector<MissingIncludeDiagInfo> MissingIncludes;
llvm::DenseSet<IncludeStructure::HeaderID> Used;
trace::Span Tracer("include_cleaner::walkUsed");
Index: clang-tools-extra/clangd/CollectMacros.h
===================================================================
--- clang-tools-extra/clangd/CollectMacros.h
+++ clang-tools-extra/clangd/CollectMacros.h
@@ -24,6 +24,8 @@
// 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,17 +45,16 @@
/// - 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 SourceManager &SM,
+ const Preprocessor &PP, MainFileMacros &Out)
+ : SM(SM), PP(PP), Out(Out) {}
void FileChanged(SourceLocation Loc, FileChangeReason,
SrcMgr::CharacteristicKind, FileID) override {
InMainFile = isInsideMainFile(Loc, SM);
}
- 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 {
@@ -68,17 +69,20 @@
void Ifdef(SourceLocation Loc, const Token &MacroName,
const MacroDefinition &MD) override {
- add(MacroName, MD.getMacroInfo());
+ add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false,
+ /*InConditionalDirective=*/true);
}
void Ifndef(SourceLocation Loc, const Token &MacroName,
const MacroDefinition &MD) override {
- add(MacroName, MD.getMacroInfo());
+ add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false,
+ /*InConditionalDirective=*/true);
}
void Defined(const Token &MacroName, const MacroDefinition &MD,
SourceRange Range) override {
- add(MacroName, MD.getMacroInfo());
+ add(MacroName, MD.getMacroInfo(), /*IsDefinition=*/false,
+ /*InConditionalDirective=*/true);
}
void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override {
@@ -91,8 +95,9 @@
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;
};
Index: clang-tools-extra/clangd/CollectMacros.cpp
===================================================================
--- clang-tools-extra/clangd/CollectMacros.cpp
+++ 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,9 @@
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});
}
class CollectPragmaMarks : public PPCallbacks {
@@ -58,5 +59,24 @@
return std::make_unique<CollectPragmaMarks>(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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
