[find-all-symbols] Add IWYU private pragma support.

[hokein]

Fix a nit.

[klimek]

include-fixer/find-all-symbols/FindAllSymbols.cpp

204–213

It seems weird to add this and just forward the interface.

include-fixer/find-all-symbols/FindAllSymbols.h

50

I think the fact that those are generated via IWYU comments are an implementation detail, and this code doesn't care. Perhaps call it HeaderMapping or HeaderRemapping. Also, it's unclear what the semantics are, so I think this needs a comment.

include-fixer/find-all-symbols/PragmaCommentHandler.h

24

I'd pull out an interface like HeaderMapCollector or ForwardingHeaderCollector, or even just pass in a std::function that collects the header. Or, just make this PragmaCommentHandler have a method to return a list of forwarding headers?
Generally, I think this couples the two classes much more than necessary.

[kimgr]

Re semantics, you may want to link to IWYU's docs at
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md
.

• Kim

Den 3 maj 2016 6:34 em skrev "Manuel Klimek via cfe-commits" <
[e-mail]>:

klimek added inline comments.

Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:173-182
@@ -165,1 +172,12 @@

+void FindAllSymbols::addPragmaHeader(FileID ID, llvm::StringRef FilePath)
{
+ PragmaHeaderMap[ID] = FilePath;
+}
+
+llvm::Optional<std::string> FindAllSymbols::getPragmaHeader(FileID ID)
const {
+ auto It = PragmaHeaderMap.find(ID);
+ if (It == PragmaHeaderMap.end())
+ return llvm::None;
+ return It->second;
+}

+

It seems weird to add this and just forward the interface.

Comment at: include-fixer/find-all-symbols/FindAllSymbols.h:49
@@ -45,1 +48,3 @@

+ void addPragmaHeader(FileID ID, llvm::StringRef FilePath);

+

I think the fact that those are generated via IWYU comments are an
implementation detail, and this code doesn't care. Perhaps call it
HeaderMapping or HeaderRemapping. Also, it's unclear what the semantics
are, so I think this needs a comment.

Comment at: include-fixer/find-all-symbols/PragmaCommentHandler.h:23
@@ +22,3 @@
+public:
+ PragmaCommentHandler(FindAllSymbols *Matcher) : Matcher(Matcher) {}

+

I'd pull out an interface like HeaderMapCollector or
ForwardingHeaderCollector, or even just pass in a std::function that
collects the header. Or, just make this PragmaCommentHandler have a method
to return a list of forwarding headers?
Generally, I think this couples the two classes much more than necessary.

Repository:

rL LLVM

http://reviews.llvm.org/D19816

---

cfe-commits mailing list
[e-mail]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[hokein]

Address review comments.

[hokein]

Improvements based on offline discussion.

[hokein]

Fix a nit.

[klimek]

include-fixer/find-all-symbols/FindAllSymbols.h

45

I'd pass in the HeaderMap here.

include-fixer/find-all-symbols/PragmaCommentHandler.h

23

"PragmaCommentHandler handle all pragma comment"
This comment doesn't really tell me anything new.

Perhaps:
PragmaCommentHandler parses comments on include files to determine when we should include a different header from the header that directly defines a symbol.

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

374

Any reason for the extra block here?

[hokein]

Update.

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

374

Because I want to keep consistence with other test cases here.

[klimek]

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

374

Why are the other test cases using an extra block?

[hokein]

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

374

I will update the code after D19913 gets submitted.

[bkramer]

include-fixer/find-all-symbols/FindAllSymbols.h

16

No space between #includes in LLVM.

include-fixer/find-all-symbols/PragmaCommentHandler.cpp

28

Isn't this regex just a complex way to write Text.find(IWYUPragma)?

[hokein]

Address comments.

[hokein]

include-fixer/find-all-symbols/PragmaCommentHandler.cpp

28

Oh, didn't notice that. Yeah, we can use the find to handle the case.for now.

[hokein]

Rebase to master.

[hokein]

Rebase.

[klimek]

LG with nit to fix.

include-fixer/find-all-symbols/FindAllSymbols.h

44–45

Add a comment describing what the Collector is for.

[hokein]

Add comments.

[hokein]

Rebase.

include-fixer/find-all-symbols/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(findAllSymbols
   FindAllSymbols.cpp
+  PragmaCommentHandler.cpp
   SymbolInfo.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend
   clangTooling
   clangToolingCore
   )
 
 add_subdirectory(tool)

include-fixer/find-all-symbols/FindAllSymbols.h

 //===-- FindAllSymbols.h - find all symbols----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_SYMBOL_MATCHER_H
 #define LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_SYMBOL_MATCHER_H
 
 #include "SymbolInfo.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include <string>
 
 namespace clang {
 namespace find_all_symbols {
 
+class HeaderMapCollector;
+
 /// \brief FindAllSymbols collects all classes, free standing functions and
 /// global variables with some extra information such as the path of the header
 /// file, the namespaces they are contained in, the type of variables and the
 /// parameter types of functions.
 ///
 /// NOTE:
 ///   - Symbols declared in main files are not collected since they can not be
 ///   included.
 ///   - Member functions are not collected because accessing them must go
 ///   through the class. #include fixer only needs the class name to find
 ///   headers.
 ///
 class FindAllSymbols : public clang::ast_matchers::MatchFinder::MatchCallback {
 public:
   class ResultReporter {
   public:
     virtual ~ResultReporter() = default;
 
     virtual void reportResult(llvm::StringRef FileName,
                               const SymbolInfo &Symbol) = 0;
   };
 
-  explicit FindAllSymbols(ResultReporter *Reporter) : Reporter(Reporter) {}
+  explicit FindAllSymbols(ResultReporter *Reporter,
+                          HeaderMapCollector *Collector)
+      : Reporter(Reporter), Collector(Collector) {}
 
   void registerMatchers(clang::ast_matchers::MatchFinder *MatchFinder);
 
   void
   run(const clang::ast_matchers::MatchFinder::MatchResult &result) override;
 
 private:
   ResultReporter *const Reporter;
+  HeaderMapCollector *const Collector;
 };
 
 } // namespace find_all_symbols
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_SYMBOL_MATCHER_H

include-fixer/find-all-symbols/FindAllSymbols.cpp

 //===-- FindAllSymbols.cpp - find all symbols -----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "FindAllSymbols.h"
+#include "HeaderMapCollector.h"
 #include "SymbolInfo.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/Optional.h"
if (const auto *NSD = dyn_cast<NamespaceDecl>(Context)) {
       const auto *RD = cast<RecordDecl>(Context);
       Contexts.emplace_back(SymbolInfo::ContextType::Record,
                             RD->getName().str());
     }
   }
   return Contexts;
 }
 
-llvm::Optional<SymbolInfo> CreateSymbolInfo(const NamedDecl *ND,
-                                            const SourceManager &SM) {
+llvm::Optional<SymbolInfo>
+CreateSymbolInfo(const NamedDecl *ND, const SourceManager &SM,
+                 const HeaderMapCollector::HeaderMap &HeaderMappingTable) {
   SymbolInfo::SymbolKind Type;
   if (llvm::isa<VarDecl>(ND)) {
     Type = SymbolInfo::SymbolKind::Variable;
   } else if (llvm::isa<FunctionDecl>(ND)) {
     Type = SymbolInfo::SymbolKind::Function;
   } else if (llvm::isa<TypedefNameDecl>(ND)) {
     Type = SymbolInfo::SymbolKind::TypedefName;
   } else if (llvm::isa<EnumConstantDecl>(ND)) {
llvm::errs() << "Declaration " << ND->getNameAsString() << "("
                  << ND->getDeclKindName()
                  << ") has invalid declaration location.";
     return llvm::None;
   }
   llvm::StringRef FilePath = SM.getFilename(Loc);
   if (FilePath.empty())
     return llvm::None;
 
+  // Check pragma remapping header.
+  auto Iter = HeaderMappingTable.find(FilePath);
+  if (Iter != HeaderMappingTable.end())
+    FilePath = Iter->second;
+
   return SymbolInfo(ND->getNameAsString(), Type, FilePath.str(),
                     SM.getExpansionLineNumber(Loc), GetContexts(ND));
 }
 
 } // namespace
 
 void FindAllSymbols::registerMatchers(MatchFinder *MatchFinder) {
   // FIXME: Handle specialization.
MatchFinder->addMatcher(
       enumConstantDecl(
           CommonFilter,
           unless(isInScopedEnum()),
           anyOf(hasDeclContext(enumDecl(HasNSOrTUCtxMatcher)), ExternCMatcher))
           .bind("decl"),
       this);
 }
 
 void FindAllSymbols::run(const MatchFinder::MatchResult &Result) {
   // Ignore Results in failing TUs.
   if (Result.Context->getDiagnostics().hasErrorOccurred()) {
     return;
   }
 
   const NamedDecl *ND = Result.Nodes.getNodeAs<NamedDecl>("decl");
   assert(ND && "Matched declaration must be a NamedDecl!");
   const SourceManager *SM = Result.SourceManager;
 
-  llvm::Optional<SymbolInfo> Symbol = CreateSymbolInfo(ND, *SM);
+  llvm::Optional<SymbolInfo> Symbol =
+      CreateSymbolInfo(ND, *SM, Collector->getHeaderMappingTable());
   if (Symbol)
     Reporter->reportResult(
         SM->getFileEntryForID(SM->getMainFileID())->getName(), *Symbol);
 }
 
 } // namespace find_all_symbols
 } // namespace clang

include-fixer/find-all-symbols/HeaderMapCollector.h

+//===-- HeaderMapCoolector.h - find all symbols------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_HEADER_MAP_COLLECTOR_H
+#define LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_HEADER_MAP_COLLECTOR_H
+
+#include "llvm/ADT/StringMap.h"
+#include <string>
+
+namespace clang {
+namespace find_all_symbols {
+
+/// \brief HeaderMappCollector collects all remapping header files.
+class HeaderMapCollector {
+public:
+  typedef llvm::StringMap<std::string> HeaderMap;
+
+  void addHeaderMapping(llvm::StringRef OrignalHeaderPath,
+                        llvm::StringRef MappingHeaderPath) {
+    HeaderMappingTable[OrignalHeaderPath] = MappingHeaderPath;
+  };
+  const HeaderMap &getHeaderMappingTable() { return HeaderMappingTable; };
+
+private:
+  /// A string-to-string map saving the mapping relationship.
+  HeaderMap HeaderMappingTable;
+};
+
+} // namespace find_all_symbols
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_HEADER_MAP_COLLECTOR_H

include-fixer/find-all-symbols/PragmaCommentHandler.h

+//===-- PragmaCommentHandler.h - find all symbols----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_PRAGMA_COMMENT_HANDLER_H
+#define LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_PRAGMA_COMMENT_HANDLER_H
+
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Preprocessor.h"
+#include <map>
+
+namespace clang {
+namespace find_all_symbols {
+
+class HeaderMapCollector;
+
+/// \brief PragmaCommentHandler parses pragma comment on include files to
+/// determine when we should include a different header from the header that
+/// directly defines a symbol.
+///
+/// Currently it only supports IWYU private pragma:
+/// https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-private
+class PragmaCommentHandler : public clang::CommentHandler {
+public:
+  PragmaCommentHandler(HeaderMapCollector *Collector) : Collector(Collector) {}
+
+  bool HandleComment(Preprocessor &PP, SourceRange Range) override;
+
+private:
+  HeaderMapCollector *const Collector;
+};
+
+} // namespace find_all_symbols
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_FIND_ALL_SYMBOLS_PRAGMA_COMMENT_HANDLER_H

include-fixer/find-all-symbols/PragmaCommentHandler.cpp

+//===-- PragmaCommentHandler.cpp - find all symbols -----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "PragmaCommentHandler.h"
+#include "FindAllSymbols.h"
+#include "HeaderMapCollector.h"
+#include "clang/Lex/Preprocessor.h"
+#include "llvm/Support/Regex.h"
+
+namespace clang {
+namespace find_all_symbols {
+namespace {
+const char IWYUPragma[] = "// IWYU pragma: private, include ";
+} // namespace
+
+bool PragmaCommentHandler::HandleComment(Preprocessor &PP, SourceRange Range) {
+  StringRef Text =
+      Lexer::getSourceText(CharSourceRange::getCharRange(Range),
+                           PP.getSourceManager(), PP.getLangOpts());
+  size_t Pos = Text.find(IWYUPragma);
+  if (Pos == StringRef::npos)
+    return false;
+  StringRef RemappingFilePath = Text.substr(Pos + std::strlen(IWYUPragma));
+  Collector->addHeaderMapping(
+      PP.getSourceManager().getFilename(Range.getBegin()),
+      RemappingFilePath.trim("\"<>"));
+  return false;
+}
+
+} // namespace find_all_symbols
+} // namespace clang

include-fixer/find-all-symbols/tool/FindAllSymbolsMain.cpp

 //===-- FindAllSymbolsMain.cpp --------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "FindAllSymbols.h"
+#include "HeaderMapCollector.h"
+#include "PragmaCommentHandler.h"
 #include "SymbolInfo.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
for (const auto &Symbol : Symbols) {
       WriteSymbolInfosToStream(OS, Symbol.second);
     }
   }
 
 private:
   std::map<std::string, std::set<SymbolInfo>> Symbols;
 };
 
+class FindAllSymbolsAction : public clang::ASTFrontendAction {
+public:
+  FindAllSymbolsAction()
+      : Reporter(), MatchFinder(), Collector(), Handler(&Collector),
+        Matcher(&Reporter, &Collector) {
+    Matcher.registerMatchers(&MatchFinder);
+  }
+
+  std::unique_ptr<clang::ASTConsumer>
+  CreateASTConsumer(clang::CompilerInstance &Compiler,
+                    StringRef InFile) override {
+    Compiler.getPreprocessor().addCommentHandler(&Handler);
+    return MatchFinder.newASTConsumer();
+  }
+
+  void EndSourceFileAction() override { Reporter.Write(OutputDir); }
+
+private:
+  YamlReporter Reporter;
+  clang::ast_matchers::MatchFinder MatchFinder;
+  HeaderMapCollector Collector;
+  PragmaCommentHandler Handler;
+  FindAllSymbols Matcher;
+};
+
 bool Merge(llvm::StringRef MergeDir, llvm::StringRef OutputFile) {
   std::error_code EC;
   std::set<SymbolInfo> UniqueSymbols;
   std::mutex SymbolMutex;
   auto AddSymbols = [&](ArrayRef<SymbolInfo> Symbols) {
     // Synchronize set accesses.
     std::unique_lock<std::mutex> LockGuard(SymbolMutex);
     UniqueSymbols.insert(Symbols.begin(), Symbols.end());
int main(int argc, const char **argv) {
   if (sources.empty()) {
     llvm::errs() << "Must specify at least one one source file.\n";
     return 1;
   }
   if (!MergeDir.empty()) {
     clang::find_all_symbols::Merge(MergeDir, sources[0]);
     return 0;
   }
-
-  clang::find_all_symbols::YamlReporter Reporter;
-  clang::find_all_symbols::FindAllSymbols Matcher(&Reporter);
-  clang::ast_matchers::MatchFinder MatchFinder;
-  Matcher.registerMatchers(&MatchFinder);
-  Tool.run(newFrontendActionFactory(&MatchFinder).get());
-  Reporter.Write(OutputDir);
+  Tool.run(
+      newFrontendActionFactory<clang::find_all_symbols::FindAllSymbolsAction>()
+          .get());
   return 0;
 }

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp

 //===-- FindAllSymbolsTests.cpp - find all symbols unit tests -------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "FindAllSymbols.h"
+#include "HeaderMapCollector.h"
+#include "PragmaCommentHandler.h"
 #include "SymbolInfo.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "gtest/gtest.h"
 #include <memory>
 #include <string>
bool hasSymbol(const SymbolInfo &Symbol) {
     }
     return false;
   }
 
 private:
   std::vector<SymbolInfo> Symbols;
 };
 
+class TestFindAllSymbolsAction : public clang::ASTFrontendAction {
+public:
+  TestFindAllSymbolsAction(FindAllSymbols::ResultReporter *Reporter)
+      : MatchFinder(), Collector(), Handler(&Collector),
+        Matcher(Reporter, &Collector) {
+    Matcher.registerMatchers(&MatchFinder);
+  }
+
+  std::unique_ptr<clang::ASTConsumer>
+  CreateASTConsumer(clang::CompilerInstance &Compiler,
+                    StringRef InFile) override {
+    Compiler.getPreprocessor().addCommentHandler(&Handler);
+    return MatchFinder.newASTConsumer();
+  }
+
+private:
+  ast_matchers::MatchFinder MatchFinder;
+  HeaderMapCollector Collector;
+  PragmaCommentHandler Handler;
+  FindAllSymbols Matcher;
+};
+
+class TestFindAllSymbolsActionFactory
+    : public clang::tooling::FrontendActionFactory {
+public:
+  TestFindAllSymbolsActionFactory(MockReporter *Reporter)
+      : Reporter(Reporter) {}
+  clang::FrontendAction *create() override {
+    return new TestFindAllSymbolsAction(Reporter);
+  }
+
+private:
+  MockReporter *const Reporter;
+};
+
 class FindAllSymbolsTest : public ::testing::Test {
 public:
   bool hasSymbol(const SymbolInfo &Symbol) {
     return Reporter.hasSymbol(Symbol);
   }
 
   bool runFindAllSymbols(StringRef Code) {
-    FindAllSymbols matcher(&Reporter);
-    clang::ast_matchers::MatchFinder MatchFinder;
-    matcher.registerMatchers(&MatchFinder);
-
     llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
         new vfs::InMemoryFileSystem);
     llvm::IntrusiveRefCntPtr<FileManager> Files(
         new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
     std::string FileName = "symbol.cc";
-    std::unique_ptr<clang::tooling::FrontendActionFactory> Factory =
-        clang::tooling::newFrontendActionFactory(&MatchFinder);
+
+    std::unique_ptr<clang::tooling::FrontendActionFactory> Factory(
+        new TestFindAllSymbolsActionFactory(&Reporter));
+
     tooling::ToolInvocation Invocation(
         {std::string("find_all_symbols"), std::string("-fsyntax-only"),
          std::string("-std=c++11"), FileName},
         Factory->create(), Files.get(),
         std::make_shared<PCHContainerOperations>());
 
     InMemoryFileSystem->addFile(HeaderName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(Code));
TEST_F(FindAllSymbolsTest, EnumTest) {
   EXPECT_FALSE(hasSymbol(Symbol));
 
   Symbol = SymbolInfo("X1", SymbolInfo::SymbolKind::EnumDecl, HeaderName, 7,
                       {{SymbolInfo::ContextType::EnumDecl, "A_ENUM"},
                        {SymbolInfo::ContextType::Record, "A"}});
   EXPECT_FALSE(hasSymbol(Symbol));
 }
 
+TEST_F(FindAllSymbolsTest, IWYUPrivatePragmaTest) {
+  static const char Code[] = R"(
+    // IWYU pragma: private, include "bar.h"
+    struct Bar {
+    };
+  )";
+  runFindAllSymbols(Code);
+
+  SymbolInfo Symbol =
+      CreateSymbolInfo("Bar", SymbolInfo::Class, "bar.h", 3, {});
+  EXPECT_TRUE(hasSymbol(Symbol));
+}
+
 } // namespace find_all_symbols
 } // namespace clang