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 <ivan.donchevskii@qt.io>

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<clang::Preprocessor> &&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 &macroNameToken,
                       const clang::MacroDirective *macroDirective) override
     {
-        addMacroAsSymbol(macroNameToken,
+        addMacroAsSymbol(macroNameToken, firstMacroInfo(macroDirective), SymbolType::MacroDefinition);
-                         firstMacroInfo(macroDirective),
-                         SymbolType::MacroDefinition);
     }
 
     void MacroUndefined(const clang::Token &macroNameToken,
@@ -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 &macroNameToken,
                        const clang::MacroDefinition &macroDefinition)
     {
         clang::MacroInfo *macroInfo = macroDefinition.getMacroInfo();
-        if (macroInfo) {
         UsedDefine usedDefine{macroNameToken.getIdentifierInfo()->getName(),
                               filePathId(macroNameToken.getLocation())};
-            auto found = std::lower_bound(m_usedDefines.begin(),
+        if (macroInfo)
-                                          m_usedDefines.end(), usedDefine);
+            addUsedDefine(std::move(usedDefine), m_usedDefines);
-
+        else
-            if (found == m_usedDefines.end() || *found != usedDefine)
+            addUsedDefine(std::move(usedDefine), m_maybeUsedDefines);
-                m_usedDefines.insert(found, std::move(usedDefine));
-        }
     }
 
     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<clang::Preprocessor> m_preprocessor;
     SymbolEntries &m_symbolEntries;
     SourceLocationEntries &m_sourceLocationEntries;
     FilePathIds &m_sourceFiles;

src/tools/clangrefactoringbackend/source/collectmacrossourcefilecallbacks.cpp

@@ -33,12 +33,14 @@ namespace ClangBackEnd {
 
 bool CollectMacrosSourceFileCallbacks::handleBeginSource(clang::CompilerInstance &compilerInstance)
 {
-    auto callbacks = std::make_unique<CollectMacrosPreprocessorCallbacks>(m_symbolEntries,
+    auto callbacks = std::make_unique<CollectMacrosPreprocessorCallbacks>(
+                m_symbolEntries,
                 m_sourceLocationEntries,
                 m_sourceFiles,
                 m_usedDefines,
                 m_filePathCache,
-                                                                          compilerInstance.getSourceManager());
+                compilerInstance.getSourceManager(),
+                compilerInstance.getPreprocessorPtr());
 
     compilerInstance.getPreprocessorPtr()->addPPCallbacks(std::move(callbacks));
 

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

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(
+                Contains(HasSymbolName("x")));
-                    Pair(_, Field(&SymbolEntry::symbolName, "x"))));
 }
 
 TEST_F(SymbolsCollector, SymbolMatchesLocation)
@@ -203,8 +213,7 @@ TEST_F(SymbolsCollector, DISABLED_ON_WINDOWS(CollectInUnsavedFile))
     collector.collectSymbols();
 
     ASSERT_THAT(collector.symbols(),
-                Contains(
+                Contains(HasSymbolName("function")));
-                    Pair(_, Field(&SymbolEntry::symbolName, "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")));
+}
+
 }