Some high-level comments. My brain couldn't get wrapped around the trie
structure yet though...
================
Comment at: include/clang/Tooling/CompilationDatabase.h:208-209
@@ +207,4 @@
+/// like this:
+/// /<project-root/src/<path>/<somefile>.cc (used as input for the tool)
+/// /<project-root/build/<symlink-to-src>/<path>/<somefile>.cc (stored in DB)
+///
----------------
<project-root> is missing the '>' above.
================
Comment at: include/clang/Tooling/CompilationDatabase.h:236-239
@@ +235,6 @@
+ /// to 'Error'.
+ StringRef findEquivalent(const PathComparator &Comparator,
+ StringRef FileName,
+ std::string *Error,
+ unsigned ConsumedLength = 0) const;
+
----------------
Rather than a "std::string *" output parameter, I would suggest passing in a
reference to a raw_ostream. You can then build a raw_ostream around a
std::string or whatever other string object you like when calling this, and the
function can avail itself of all the streamed output conveniences.
================
Comment at: lib/Tooling/JSONCompilationDatabase.cpp:289
@@ +288,3 @@
+ llvm::SmallString<8> DirectoryStorage;
+ llvm::SmallString<128>
AbsolutePath(Directory->getValue(DirectoryStorage));
+ llvm::sys::path::append(AbsolutePath, FileName);
----------------
is this line over 80 columns, or is phab just being confusing?
================
Comment at: include/clang/Tooling/CompilationDatabase.h:200-201
@@ +199,4 @@
+
+/// \brief A trie to efficiently match against the entries of the compilation
+/// database in order of matching suffix length.
+///
----------------
You explain in nice detail why this exists and its purpose, but no where do you
really explain the datastructure technique you are using to build the trie
efficiently. What are the expected storage requirements? what's the strategy
used? I feel left a bit in the dark regarding the actual algorithms.
================
Comment at: include/clang/Tooling/CompilationDatabase.h:193-198
@@ -188,1 +192,8 @@
+struct PathComparator {
+ virtual bool equivalent(const Twine &FileA, const Twine &FileB) const {
+ return FileA.str() == FileB.str() ||
+ llvm::sys::fs::equivalent(FileA, FileB);
+ }
+};
+
----------------
Why expose this and require it to be passed into findEquivalent? It would
simplify things to sink this into the implementation details of findEquivalent.
and hide its existence from the users.
http://llvm-reviews.chandlerc.com/D30
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
