[cfe-commits] [PATCH] Make CompilationDatabase work with symlinks and relative paths

Manuel Klimek reviews at llvm-reviews.chandlerc.com
Mon Oct 8 01:07:59 PDT 2012

Comment at: include/clang/Tooling/CompilationDatabase.h:193
@@ -188,1 +192,3 @@
+struct PathComparator {
+  virtual bool equivalent(const Twine &FileA, const Twine &FileB) const {
I think the MatchTrie and PathComparator should go into a different header file - they are implementation details of the compilation databases.

Comment at: include/clang/Tooling/CompilationDatabase.h:216-224
@@ +215,11 @@
+/// For a given input file, the \c FileNameMatchTrie finds its entries in order
+/// of matching suffix length. For each suffix length, there might be one or
+/// more entries in the database. For each of those entries, it calls
+/// \c llvm::sys::fs::equivalent() (injected as \c PathComparator). There might
+/// be zero or more entries with the same matching suffix length that are
+/// equivalent to the input file. Three cases are distinguished:
+/// 0  equivalent files: Continue with the next suffix length.
+/// 1  equivalent file:  Best match found, return it.
+/// >1 equivalent files: Match is ambiguous, return error.
Here and below: the repetition of implementation details in the comment strikes me as odd.  Why is a user of this at all interested in how exactly the implementation works? (I'm also not sure that's really what Chandler was after...)

Comment at: include/clang/Tooling/CompilationDatabase.h:256
@@ +255,3 @@
+  /// \brief Inserts 'NewPath' into this trie.
+  void insert(StringRef NewPath, unsigned ConsumedLength = 0);
What's ConsumedLength?

Comment at: include/clang/Tooling/CompilationDatabase.h:266
@@ +265,3 @@
+  StringRef findEquivalent(StringRef FileName,
+                           llvm::raw_ostream &Error) const;
Now might be the time to start using Diagnostics instead of spreading the use of other error mechanisms...

Comment at: include/clang/Tooling/CompilationDatabase.h:275
@@ +274,3 @@
+  /// recursion.
+  StringRef findEquivalent(const PathComparator &Comparator,
+                           StringRef FileName,
I'd want to put the PathComparator in as a template parameter, and try to get rid of the friend test.

Comment at: lib/Tooling/CompilationDatabase.cpp:136
@@ +135,3 @@
+void FileNameMatchTrie::insert(StringRef NewPath, unsigned ConsumedLength) {
+  if (llvm::sys::path::is_relative(NewPath))
+    return;
Why do we need this? (as in: a comment might help :)


More information about the cfe-commits mailing list