From 4f0c17bc5d1a7233ca2803002ddfec2277c69feb Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Tue, 23 Jan 2018 14:10:17 +0100 Subject: [PATCH] Clang: Improve Macro collection Now undefined macros are added to used macros but we filter out header guards and dynamic libraries export. The export is quite simple but without we would think that exports change the AST. Change-Id: Ic16f60a6675e397dfd769c53caf77056708d8459 Reviewed-by: Ivan Donchevskii --- .../collectmacrospreprocessorcallbacks.h | 76 +++++++++++++--- .../collectmacrossourcefilecallbacks.cpp | 14 +-- .../unittest/data/symbolscollector_defines.h | 6 ++ tests/unit/unittest/symbolscollector-test.cpp | 90 +++++++++++++++++-- 4 files changed, 161 insertions(+), 25 deletions(-) diff --git a/src/tools/clangrefactoringbackend/source/collectmacrospreprocessorcallbacks.h b/src/tools/clangrefactoringbackend/source/collectmacrospreprocessorcallbacks.h index d72691a70a7..62a406f0da2 100644 --- a/src/tools/clangrefactoringbackend/source/collectmacrospreprocessorcallbacks.h +++ b/src/tools/clangrefactoringbackend/source/collectmacrospreprocessorcallbacks.h @@ -49,8 +49,10 @@ public: FilePathIds &sourceFiles, UsedDefines &usedDefines, FilePathCachingInterface &filePathCache, - const clang::SourceManager &sourceManager) + const clang::SourceManager &sourceManager, + std::shared_ptr &&preprocessor) : SymbolsVisitorBase(filePathCache, sourceManager), + m_preprocessor(std::move(preprocessor)), m_symbolEntries(symbolEntries), m_sourceLocationEntries(sourceLocationEntries), m_sourceFiles(sourceFiles), @@ -114,9 +116,7 @@ public: void MacroDefined(const clang::Token ¯oNameToken, const clang::MacroDirective *macroDirective) override { - addMacroAsSymbol(macroNameToken, - firstMacroInfo(macroDirective), - SymbolType::MacroDefinition); + addMacroAsSymbol(macroNameToken, firstMacroInfo(macroDirective), SymbolType::MacroDefinition); } void MacroUndefined(const clang::Token ¯oNameToken, @@ -139,19 +139,66 @@ public: SymbolType::MacroUsage); } + void EndOfMainFile() override + { + filterOutHeaderGuards(); + mergeUsedDefines(); + filterOutExports(); + } + + void filterOutHeaderGuards() + { + auto partitionPoint = std::stable_partition(m_maybeUsedDefines.begin(), + m_maybeUsedDefines.end(), + [&] (const UsedDefine &usedDefine) { + llvm::StringRef id{usedDefine.defineName.data(), usedDefine.defineName.size()}; + clang::IdentifierInfo &identifierInfo = m_preprocessor->getIdentifierTable().get(id); + clang::MacroInfo *macroInfo = m_preprocessor->getMacroInfo(&identifierInfo); + return !macroInfo || !macroInfo->isUsedForHeaderGuard(); + }); + + m_maybeUsedDefines.erase(partitionPoint, m_maybeUsedDefines.end()); + } + + void filterOutExports() + { + auto partitionPoint = std::stable_partition(m_usedDefines.begin(), + m_usedDefines.end(), + [&] (const UsedDefine &usedDefine) { + return !usedDefine.defineName.contains("EXPORT"); + }); + + m_usedDefines.erase(partitionPoint, m_usedDefines.end()); + } + + void mergeUsedDefines() + { + m_usedDefines.reserve(m_usedDefines.size() + m_maybeUsedDefines.size()); + auto insertionPoint = m_usedDefines.insert(m_usedDefines.end(), + m_maybeUsedDefines.begin(), + m_maybeUsedDefines.end()); + std::inplace_merge(m_usedDefines.begin(), insertionPoint, m_usedDefines.end()); + } + + static void addUsedDefine(UsedDefine &&usedDefine, UsedDefines &usedDefines) + { + auto found = std::lower_bound(usedDefines.begin(), + usedDefines.end(), usedDefine); + + if (found == usedDefines.end() || *found != usedDefine) + usedDefines.insert(found, std::move(usedDefine)); + } + void addUsedDefine(const clang::Token ¯oNameToken, const clang::MacroDefinition ¯oDefinition) { clang::MacroInfo *macroInfo = macroDefinition.getMacroInfo(); - if (macroInfo) { - UsedDefine usedDefine{macroNameToken.getIdentifierInfo()->getName(), - filePathId(macroNameToken.getLocation())}; - auto found = std::lower_bound(m_usedDefines.begin(), - m_usedDefines.end(), usedDefine); - - if (found == m_usedDefines.end() || *found != usedDefine) - m_usedDefines.insert(found, std::move(usedDefine)); - } + UsedDefine usedDefine{macroNameToken.getIdentifierInfo()->getName(), + filePathId(macroNameToken.getLocation())}; + if (macroInfo) + addUsedDefine(std::move(usedDefine), m_usedDefines); + else + addUsedDefine(std::move(usedDefine), m_maybeUsedDefines); } static const clang::MacroInfo *firstMacroInfo(const clang::MacroDirective *macroDirective) @@ -176,7 +223,6 @@ public: clang::SourceLocation sourceLocation = macroNameToken.getLocation(); if (macroInfo && sourceLocation.isFileID()) { FilePathId fileId = filePathId(sourceLocation); - auto macroName = macroNameToken.getIdentifierInfo()->getName(); if (fileId.isValid()) { auto macroName = macroNameToken.getIdentifierInfo()->getName(); SymbolIndex globalId = toSymbolIndex(macroInfo); @@ -212,6 +258,8 @@ public: } private: + UsedDefines m_maybeUsedDefines; + std::shared_ptr m_preprocessor; SymbolEntries &m_symbolEntries; SourceLocationEntries &m_sourceLocationEntries; FilePathIds &m_sourceFiles; diff --git a/src/tools/clangrefactoringbackend/source/collectmacrossourcefilecallbacks.cpp b/src/tools/clangrefactoringbackend/source/collectmacrossourcefilecallbacks.cpp index ad147a94cfd..317d93bdc3c 100644 --- a/src/tools/clangrefactoringbackend/source/collectmacrossourcefilecallbacks.cpp +++ b/src/tools/clangrefactoringbackend/source/collectmacrossourcefilecallbacks.cpp @@ -33,12 +33,14 @@ namespace ClangBackEnd { bool CollectMacrosSourceFileCallbacks::handleBeginSource(clang::CompilerInstance &compilerInstance) { - auto callbacks = std::make_unique(m_symbolEntries, - m_sourceLocationEntries, - m_sourceFiles, - m_usedDefines, - m_filePathCache, - compilerInstance.getSourceManager()); + auto callbacks = std::make_unique( + m_symbolEntries, + m_sourceLocationEntries, + m_sourceFiles, + m_usedDefines, + m_filePathCache, + compilerInstance.getSourceManager(), + compilerInstance.getPreprocessorPtr()); compilerInstance.getPreprocessorPtr()->addPPCallbacks(std::move(callbacks)); diff --git a/tests/unit/unittest/data/symbolscollector_defines.h b/tests/unit/unittest/data/symbolscollector_defines.h index 91ee38cdf98..6cb2ee91ef9 100644 --- a/tests/unit/unittest/data/symbolscollector_defines.h +++ b/tests/unit/unittest/data/symbolscollector_defines.h @@ -33,4 +33,10 @@ void foo(MACRO_EXPANSION); #undef UN_DEFINE +#define CLASS_EXPORT + +struct CLASS_EXPORT Foo +{ +}; + #endif // SYMBOLSCOLLECTOR_DEFINES_H diff --git a/tests/unit/unittest/symbolscollector-test.cpp b/tests/unit/unittest/symbolscollector-test.cpp index b04464521ea..777d48b38d6 100644 --- a/tests/unit/unittest/symbolscollector-test.cpp +++ b/tests/unit/unittest/symbolscollector-test.cpp @@ -83,6 +83,17 @@ MATCHER_P2(HasLineColumn, line, column, && entry.lineColumn.column == column; } +MATCHER_P(HasSymbolName, symbolName, + std::string(negation ? "hasn't" : "has") + + " symbol name: " + + symbolName + ) +{ + const SymbolEntry &entry = arg.second; + + return entry.symbolName == symbolName; +} + class SymbolsCollector : public testing::Test { protected: @@ -115,8 +126,7 @@ TEST_F(SymbolsCollector, CollectSymbolName) collector.collectSymbols(); ASSERT_THAT(collector.symbols(), - Contains( - Pair(_, Field(&SymbolEntry::symbolName, "x")))); + Contains(HasSymbolName("x"))); } TEST_F(SymbolsCollector, SymbolMatchesLocation) @@ -203,8 +213,7 @@ TEST_F(SymbolsCollector, DISABLED_ON_WINDOWS(CollectInUnsavedFile)) collector.collectSymbols(); ASSERT_THAT(collector.symbols(), - Contains( - Pair(_, Field(&SymbolEntry::symbolName, "function")))); + Contains(HasSymbolName("function"))); } TEST_F(SymbolsCollector, SourceFiles) @@ -297,7 +306,7 @@ TEST_F(SymbolsCollector, DontCollectSourceFilesAfterFilesAreCleared) ASSERT_THAT(collector.sourceFiles(), IsEmpty()); } -TEST_F(SymbolsCollector, CollectDefines) +TEST_F(SymbolsCollector, CollectUsedDefinesWithExternalDefine) { auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); collector.addFiles({fileId}, {"cc", "-DCOMPILER_ARGUMENT"}); @@ -313,6 +322,44 @@ TEST_F(SymbolsCollector, CollectDefines) Eq(UsedDefine{"COMPILER_ARGUMENT", fileId}))); } +TEST_F(SymbolsCollector, CollectUsedDefinesWithoutExternalDefine) +{ + auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); + collector.addFiles({fileId}, {"cc"}); + + collector.collectSymbols(); + + ASSERT_THAT(collector.usedDefines(), + ElementsAre(Eq(UsedDefine{"DEFINED", fileId}), + Eq(UsedDefine{"IF_DEFINE", fileId}), + Eq(UsedDefine{"__clang__", fileId}), + Eq(UsedDefine{"IF_NOT_DEFINE", fileId}), + Eq(UsedDefine{"MACRO_EXPANSION", fileId}), + Eq(UsedDefine{"COMPILER_ARGUMENT", fileId}))); +} + +TEST_F(SymbolsCollector, DontCollectHeaderGuards) +{ + auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); + collector.addFiles({fileId}, {"cc"}); + + collector.collectSymbols(); + + ASSERT_THAT(collector.usedDefines(), + Not(Contains(Eq(UsedDefine{"SYMBOLSCOLLECTOR_DEFINES_H", fileId})))); +} + +TEST_F(SymbolsCollector, DontCollectDynamicLibraryExports) +{ + auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); + collector.addFiles({fileId}, {"cc"}); + + collector.collectSymbols(); + + ASSERT_THAT(collector.usedDefines(), + Not(Contains(Eq(UsedDefine{"CLASS_EXPORT", fileId})))); +} + TEST_F(SymbolsCollector, CollectMacroDefinitionSourceLocation) { auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); @@ -412,4 +459,37 @@ TEST_F(SymbolsCollector, CollectMacroUsageBuiltInSourceLocation) Contains(IsSourceLocationEntry(symbolId("__clang__"), fileId, 29, 9, SymbolType::MacroUsage))); } +TEST_F(SymbolsCollector, CollectMacroDefinitionSymbols) +{ + auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); + collector.addFiles({fileId}, {"cc"}); + + collector.collectSymbols(); + + ASSERT_THAT(collector.symbols(), + Contains(HasSymbolName("IF_NOT_DEFINE"))); +} + +TEST_F(SymbolsCollector, CollectMacroBuiltInSymbols) +{ + auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); + collector.addFiles({fileId}, {"cc"}); + + collector.collectSymbols(); + + ASSERT_THAT(collector.symbols(), + Contains(HasSymbolName("__clang__"))); +} + +TEST_F(SymbolsCollector, CollectMacroCompilerArgumentSymbols) +{ + auto fileId = filePathId(TESTDATA_DIR "/symbolscollector_defines.h"); + collector.addFiles({fileId}, {"cc", "-DCOMPILER_ARGUMENT"}); + + collector.collectSymbols(); + + ASSERT_THAT(collector.symbols(), + Contains(HasSymbolName("COMPILER_ARGUMENT"))); +} + }