[PATCH] D19314: [include-fixer] Add a prototype for a new include fixing tool.

[Manuel Klimek via cfe-commits]

klimek added a subscriber: klimek.

================
Comment at: include-fixer/FixedXrefsDB.h:18
@@ +17,3 @@
+
+/// Xref database with fixed content, intended for testing
+// FIXME: Move to unittest once ClangIncludeFixer doesn't depend on it anymore.
----------------
Add '.', remove "intended for testing". I don't think telling people to not use it outside of testing helps a lot. For example, in the compilation database we found that it helps to allow people to pass a fixed-compilation-db at the command line; similar things might make sense here.

================
Comment at: include-fixer/FixedXrefsDB.h:20
@@ +19,3 @@
+// FIXME: Move to unittest once ClangIncludeFixer doesn't depend on it anymore.
+class FixedXrefsDB : public XrefsDB {
+public:
----------------
I'd make it constructible from a map<string, vector<string>>. Especially for tests, I'd want to see the test setup.

================
Comment at: include-fixer/IncludeFixer.cpp:117
@@ +116,3 @@
+
+    /// If we have a scope specification use that to get more precise results.
+    std::string QueryString;
----------------
specification, *,*

================
Comment at: include-fixer/IncludeFixer.cpp:132
@@ +131,3 @@
+
+private:
+  /// Query the database for a given identifier.
----------------
Can we sort this so the public interface comes first? Also, why is the public interface so large?

================
Comment at: include-fixer/IncludeFixer.cpp:179
@@ +178,3 @@
+
+  /// \brief Rewrite the associated source file with our tentative suggestions.
+  /// \param rewriter a valid Rewriter.
----------------
This is not actually rewriting, but constructing the replacements, right?

================
Comment at: include-fixer/IncludeFixer.cpp:184-190
@@ +183,9 @@
+               std::vector<clang::tooling::Replacement> &replacements) {
+    for (const auto &ToTry : Untried) {
+      DEBUG(llvm::dbgs() << "Adding include " << ToTry << "\n");
+      std::string ToAdd = "\n#include " + ToTry;
+      // If this is the only include in the file, add the newline after it, not
+      // before.
+      if (LastIncludeOffset == 0)
+        std::rotate(ToAdd.begin(), ToAdd.begin() + 1, ToAdd.end());
+
----------------
Can we reuse functionality from clang-format here?

================
Comment at: include-fixer/IncludeFixer.h:26
@@ +25,3 @@
+  IncludeFixerActionFactory(
+      std::unique_ptr<XrefsDB> Xrefs,
+      std::vector<clang::tooling::Replacement> &Replacements);
----------------
Why are we passing ownership?

================
Comment at: include-fixer/IncludeFixer.h:36
@@ +35,3 @@
+
+  XrefsDB *getXrefsDB() const { return Xrefs.get(); }
+
----------------
That also seems weird in the interface here.


http://reviews.llvm.org/D19314