[PATCH] D19869: Added XrefsDBManager into include-fixer and made XrefsDB return SymbolInfo.

[Manuel Klimek via cfe-commits]

klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.

lg


================
Comment at: include-fixer/InMemoryXrefsDB.cpp:24-26
@@ +23,5 @@
+    for (const auto &Header : Entry.second) {
+      SymbolInfo Info;
+      Info.Name = Names.back();
+      Info.FilePath = Header;
+      for (auto IdentiferContext = Names.rbegin() + 1;
----------------
hokein wrote:
> Yeah, the intention is that it's not easy to construct a **completed** instance at one time since it has many fields. 
Overall, I think creating an instance and then setting members should only be done from places very strongly coupled to the class.
Generally, I think in these cases creating static members of the class that construct the objects from the various data sources you have (like in this case from a string, and a vector<string>) are preferable; not necessary to address this in this CL, but please add a FIXME.

(also, specifically regarding the comment that it's not easy to construct a completed instance: that is a large hint that it should be encoded somewhere close to the class)

================
Comment at: include-fixer/IncludeFixer.h:34-35
@@ -33,3 +33,4 @@
   IncludeFixerActionFactory(
-      XrefsDB &Xrefs, std::vector<clang::tooling::Replacement> &Replacements,
+      XrefsDBManager &XrefsDBMgr,
+      std::vector<clang::tooling::Replacement> &Replacements,
       bool MinimizeIncludePaths = true);
----------------
ioeric wrote:
> klimek wrote:
> > That seems unexpected. Why is the XrefsDBManager in the business of doing SymbolInfo -> include path mappings?
> Since the mapping was done in XrefsDB before, I kept the way it was. Migrating SymbolInfo -> Include path mappings into IncludeFixer will be the next step, and I'll need to discuss with Ben about where exactly we want to place it.
Please add a FIXME that this is going to change.

================
Comment at: unittests/include-fixer/IncludeFixerTest.cpp:86
@@ +85,3 @@
+  // string without "std::" can also be fixed since fixed db results go through
+  // XrefsDBManager, and XrefsDBManager matches unqualified identifier too.
+  EXPECT_EQ("#include <string>\nstring foo;\n",
----------------
identifiers


http://reviews.llvm.org/D19869