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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
