[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-22 Thread Phabricator via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL313975: Add Cross Translation Unit support library (authored by xazax). Changed prior to commit: https://reviews.llvm.org/D34512?vs=116195=116327#toc Repository: rL LLVM

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-21 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. > But I also think it wouldn't be good to block this until ClanD indexing > reaching a mature state. I agree 100%. > All in all, we are willing to reuse as much of ClangD indexing as

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-21 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 116195. xazax.hun added a comment. - Unittests now creates temporary files at the correct location - Temporary files are also cleaned up when the process is terminated - Absolute paths are handled correctly by the library https://reviews.llvm.org/D34512

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-21 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#877032, @dcoughlin wrote: > I'm OK with this going into the repo a is (although it is light on tests!), > as long as we have an agreement that you'll be OK with iteration on both the > interface and the implementation to handle

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-20 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. I'm OK with this going into the repo a is (although it is light on tests!), as long as we have an agreement that you'll be OK with iteration on both the interface and the implementation to handle real-world projects. More specifically, for this to work well on

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-15 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. Any further reviews? What are the criteria to accept this patch? Who or how many people should accept this? https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-07 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 114133. xazax.hun marked 8 inline comments as done. xazax.hun added a comment. - Address review comments. https://reviews.llvm.org/D34512 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-07 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35 +namespace { +// FIXME: This class is will be removed after the transition to llvm::Error. +class IndexErrorCategory : public std::error_category { dcoughlin wrote: > Is this

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-06 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Thanks Gabor! Some additional comments in line. Comment at: include/clang/CrossTU/CrossTranslationUnit.h:118 + /// + /// \return Returns a map with the loaded AST Units and the declarations + /// with the definitions. Is this

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-04 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:82 +size_t Pos = Line.find(" "); +StringRef LineRef{Line}; +if (Pos > 0 && Pos != std::string::npos) { danielmarjamaki wrote: > LineRef can be const StringRef is an

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-09-04 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 113740. xazax.hun marked 19 inline comments as done. xazax.hun added a comment. - Make the API capable of using custom lookup strategies for CTU - Fix review comments https://reviews.llvm.org/D34512 Files: include/clang/Basic/AllDiagnostics.h

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-31 Thread Daniel Marjamäki via Phabricator via cfe-commits
danielmarjamaki added a comment. small nits Comment at: include/clang/CrossTU/CrossTranslationUnit.h:58 + +/// \brief This function can parse an index file that determines which +///translation unit contains which definition. I suggest that "can" is

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-30 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#856821, @benlangmuir wrote: > In https://reviews.llvm.org/D34512#856301, @xazax.hun wrote: > > > In https://reviews.llvm.org/D34512#856184, @dcoughlin wrote: > > > > > In either case, the important scenario I think we should support

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-30 Thread Ben Langmuir via Phabricator via cfe-commits
benlangmuir added a comment. In https://reviews.llvm.org/D34512#856301, @xazax.hun wrote: > In https://reviews.llvm.org/D34512#856184, @dcoughlin wrote: > > > In either case, the important scenario I think we should support is > > choosing at a call site to a C function the most likely

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-30 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 113206. xazax.hun added a comment. - Added unit test to ensure the produced index format can be parsed. - Added further diagnostics. https://reviews.llvm.org/D34512 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-30 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#856184, @dcoughlin wrote: > In either case, the important scenario I think we should support is choosing > at a call site to a C function the most likely definition of the called > function, based on number and type of parameters,

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-29 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. > The importing alters the ASTContext, so the only way to choose between the > definitions would be via a callback that is triggered before the import is > done. What do you think? I think that could work. Another possibility would be to have a two step process. The

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-29 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 113088. xazax.hun marked 2 inline comments as done. xazax.hun added a comment. - Updates according to review comments. https://reviews.llvm.org/D34512 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-29 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#854729, @dcoughlin wrote: > Except for some drive-by nits, this is a high-level review. > > I have some high-level questions about the design of the library: > > 1. **How do you intend to handle the case when there are multiple

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-28 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment. Except for some drive-by nits, this is a high-level review. I have some high-level questions about the design of the library: 1. **How do you intend to handle the case when there are multiple function definitions with the same USR?** Whose responsibility it is to

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-21 Thread Daniel Krupp via Phabricator via cfe-commits
dkrupp added a subscriber: zaks.anna. dkrupp added a comment. The creation of this library (libCrossTU) is approved for importing function definitions. @zaks.anna, @NoQ , @klimek could you please help us reviewing the code itself? Then, when this is approved, we could progress with the review

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-10 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 110559. xazax.hun marked 4 inline comments as done. xazax.hun added a comment. - Address review comments https://reviews.llvm.org/D34512 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-10 Thread Whisperity via Phabricator via cfe-commits
whisperity added inline comments. Comment at: include/clang/CrossTU/CrossTranslationUnit.h:42 +/// Note that this class also implements caching. +class CrossTranslationUnit { +public: xazax.hun wrote: > whisperity wrote: > > Does the name of this class make

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-10 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#836831, @whisperity wrote: > Apart from those in the in-line comments, I have a question: how safe is this > library to `Release` builds? I know this is only a submodule dependency for > the "real deal" in

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-09 Thread Whisperity via Phabricator via cfe-commits
whisperity added a comment. Apart from those in the in-line comments, I have a question: how safe is this library to `Release` builds? I know this is only a submodule dependency for the "real deal" in https://reviews.llvm.org/D30691, but I have seen some asserts that "imported function should

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-03 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 109559. xazax.hun marked 2 inline comments as done. xazax.hun added a comment. - Address further review comments. https://reviews.llvm.org/D34512 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-03 Thread Krasimir Georgiev via Phabricator via cfe-commits
krasimir added inline comments. Comment at: include/clang/CrossTU/CrossTUDiagnostic.h:16 +namespace clang { + namespace diag { +enum { LLVM Style uses no indent for namespaces. Reformat with `clang-format`. Comment at:

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-03 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 109552. xazax.hun marked 3 inline comments as done. xazax.hun added a comment. - Addressed review comments. https://reviews.llvm.org/D34512 Files: include/clang/Basic/AllDiagnostics.h include/clang/Basic/CMakeLists.txt

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-03 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#829712, @rsmith wrote: > Organizationally, this seems fine. Carry on :) Great news! Thank you! https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-08-02 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
rsmith added a comment. Organizationally, this seems fine. Carry on :) Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:229-231 +def err_fnmap_parsing : Error< + "error parsing CrossTU index file: '%0' line: %1 'USR filename' format " + "expected">;

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-31 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#806505, @klimek wrote: > Yep, I want Richard's approval for the name. I think he already expressed a > pro-pulling-out stance. Great! Ping for the name approval. https://reviews.llvm.org/D34512

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-12 Thread Manuel Klimek via Phabricator via cfe-commits
klimek added a comment. Yep, I want Richard's approval for the name. I think he already expressed a pro-pulling-out stance. https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-12 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#803724, @klimek wrote: > Specifically, ping Richard for new top-level lib in clang. Richard proposed pulling this out into a separate library in the first place. Do we need his approval for the name? Or we want him to consider if

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-10 Thread Manuel Klimek via Phabricator via cfe-commits
klimek added a comment. Specifically, ping Richard for new top-level lib in clang. https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-10 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. gentle ping https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-06 Thread Manuel Klimek via Phabricator via cfe-commits
klimek added a comment. In https://reviews.llvm.org/D34512#800626, @whisperity wrote: > In https://reviews.llvm.org/D34512#800502, @xazax.hun wrote: > > > In https://reviews.llvm.org/D34512#800499, @klimek wrote: > > > > > In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote: > > > > > >

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-06 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 105412. xazax.hun marked an inline comment as done. xazax.hun added a comment. - Fix a copy and paste error and removed an unintended change. https://reviews.llvm.org/D34512 Files: include/clang/Basic/DiagnosticFrontendKinds.td

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-06 Thread Whisperity via Phabricator via cfe-commits
whisperity added a comment. In https://reviews.llvm.org/D34512#800502, @xazax.hun wrote: > In https://reviews.llvm.org/D34512#800499, @klimek wrote: > > > In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote: > > > > > It looks like Richard approved libTooling as a dependency for clang on

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-06 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added a comment. In https://reviews.llvm.org/D34512#800618, @klimek wrote: > +Richard as top-level code owner for new libs. > > For bikeshedding the name: I'd have liked libIndex, but that's already taken. > CrossTU doesn't seem too bad to me, too, though. Some brainstorming for the

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-06 Thread Alex Lorenz via Phabricator via cfe-commits
arphaman added inline comments. Comment at: unittests/CrossTU/CMakeLists.txt:10 + +target_link_libraries(ToolingTests + clangAST `CrossTUTests`? https://reviews.llvm.org/D34512 ___ cfe-commits mailing list

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-06 Thread Manuel Klimek via Phabricator via cfe-commits
klimek added a reviewer: rsmith. klimek added a comment. +Richard as top-level code owner for new libs. For bikeshedding the name: I'd have liked libIndex, but that's already taken. CrossTU doesn't seem too bad to me, too, though. https://reviews.llvm.org/D34512

[PATCH] D34512: Add preliminary Cross Translation Unit support library

2017-07-06 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun updated this revision to Diff 105409. xazax.hun retitled this revision from "[libTooling] Add preliminary Cross Translation Unit support for libTooling" to "Add preliminary Cross Translation Unit support library". xazax.hun added a comment. - Move CrossTU functionality into its own