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

[Daniel Jasper]

================
Comment at: include/clang/Tooling/CompilationDatabase.h:193
@@ -188,1 +192,3 @@
 
+struct PathComparator {
+  virtual bool equivalent(const Twine &FileA, const Twine &FileB) const {
----------------
Manuel Klimek wrote:
> I think the MatchTrie and PathComparator should go into a different header file - they are implementation details of the compilation databases.
Moved into a separate .h and .cpp file. I think a header file in clang/include/Tooling might still be useful as this is reusable for custom 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.
+///
----------------
Manuel Klimek wrote:
> 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...)
I agree. What can I do?

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

================
Comment at: include/clang/Tooling/CompilationDatabase.h:275
@@ +274,3 @@
+  /// recursion.
+  StringRef findEquivalent(const PathComparator &Comparator,
+                           StringRef FileName,
----------------
Manuel Klimek wrote:
> I'd want to put the PathComparator in as a template parameter, and try to get rid of the friend test.
The test can now inject it into the FileNameTrie.

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

================
Comment at: include/clang/Tooling/CompilationDatabase.h:266
@@ +265,3 @@
+  StringRef findEquivalent(StringRef FileName,
+                           llvm::raw_ostream &Error) const;
+
----------------
Manuel Klimek wrote:
> Now might be the time to start using Diagnostics instead of spreading the use of other error mechanisms...
AFAICT, this implies a somewhat significant refactoring of other parts of libTooling (outside of the CompilationDatabase). I am happy to start working on that, but I would prefer to do that as a separate patch.


http://llvm-reviews.chandlerc.com/D30