xazax.hun added a comment.
In https://reviews.llvm.org/D34512#890537, @r.stahl wrote:
> In https://reviews.llvm.org/D34512#877643, @xazax.hun wrote:
>
> > - Unittests now creates temporary files at the correct location
> > - Temporary files are also cleaned up when the process is terminated
> >
r.stahl added a comment.
In https://reviews.llvm.org/D34512#877643, @xazax.hun wrote:
> - 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
I think there
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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,
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
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
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
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
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
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
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
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
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
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
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:
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
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
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">;
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
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
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
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
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
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:
> > >
> > >
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
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
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
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
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
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
44 matches
Mail list logo
