This is an archive of the discontinued LLVM Phabricator instance.

Make CompilationDatabase work with symlinks and relative paths
ClosedPublic

Authored by djasper on Aug 27 2012, 5:43 PM.

Details

Reviewers
klimek
chandlerc
Summary

This is a first version of my patch to properly support symlinks in the compilation database design, hopefully following the algorithm we discussed on IRC (see class comment of FileNameMatchTrie).

Diff Detail

Event Timeline

Some high-level comments. My brain couldn't get wrapped around the trie structure yet though...

include/clang/Tooling/CompilationDatabase.h
190–195

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.

197–198

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.

205–206

<project-root> is missing the '>' above.

233–236

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.

lib/Tooling/JSONCompilationDatabase.cpp
288

is this line over 80 columns, or is phab just being confusing?

djasper updated this revision to Unknown Object (????).Sep 17 2012, 12:30 PM
djasper added inline comments.Sep 17 2012, 12:33 PM
include/clang/Tooling/CompilationDatabase.h
190–195

Injecting this is important for testing and I did not want to store anything in the Trie nodes. I have now split findEquivalent into two methods, the public one requiring fewer arguments. I'll put more effort into hiding PathComparator once you okayed the design in general.

197–198

How about this?

205–206

Added.

233–236

Done.

lib/Tooling/JSONCompilationDatabase.cpp
288

No, it was 81 columns.

klimek added inline comments.Oct 8 2012, 1:07 AM
include/clang/Tooling/CompilationDatabase.h
190

I think the MatchTrie and PathComparator should go into a different header file - they are implementation details of the compilation databases.

213–221

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...)

253

What's ConsumedLength?

263

Now might be the time to start using Diagnostics instead of spreading the use of other error mechanisms...

272

I'd want to put the PathComparator in as a template parameter, and try to get rid of the friend test.

lib/Tooling/CompilationDatabase.cpp
136 ↗(On Diff #113)

Why do we need this? (as in: a comment might help :)

djasper updated this revision to Unknown Object (????).Oct 8 2012, 6:19 AM
djasper added inline comments.Oct 8 2012, 6:58 AM
include/clang/Tooling/CompilationDatabase.h
190

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.

213–221

I agree. What can I do?

253

Explained in comment now.

263

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.

272

The test can now inject it into the FileNameTrie.

lib/Tooling/CompilationDatabase.cpp
136 ↗(On Diff #113)

Added comment.

klimek added a comment.Oct 8 2012, 7:08 AM

Apart from:

  • my nits
  • my disagreement about where to put the comments
  • a hunch that we'll want more tests ;)

looks good.

include/clang/Tooling/FileMatchTrie.h
105

I have a small preference to overloading the constructor (because we don't need to ever call this multiple times).

lib/Tooling/FileMatchTrie.cpp
80

80 columns.

145

80 columns.

djasper updated this revision to Unknown Object (????).Oct 8 2012, 8:12 AM

Nits and comments fixed.

include/clang/Tooling/FileMatchTrie.h
105

Done.

lib/Tooling/FileMatchTrie.cpp
80

Done.

145

Done.

djasper closed this revision.Oct 8 2012, 9:11 AM

Submitted as r165392.