[PATCH] D52084: [clangd] Improve PostingList iterator string representation

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev closed this revision.
kbobyrev added a comment.

I think this one is addressed in the VByte patch, so I'm closing this revision 
in order to prevent conflicts between these two.


https://reviews.llvm.org/D52084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52084: [clangd] Improve PostingList iterator string representation

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In https://reviews.llvm.org/D52084#1238537, @sammccall wrote:

> This change seems fine but...
>
> This representation with the raw DocIDs and position dumped seems mostly 
> useful for debugging buggy iterator implementations, but not really useful 
> for understanding query structure and behavior.


I agree; There is another issue that I was looking into: I think that it might 
be useful to understand the structure of fuzzy find query when using `dexp` 
tool and I thought that it would be great if we could dump the iterator tree 
structure along with the results (which is an extension of 
https://reviews.llvm.org/D52233). For `dexp` usecase, it would be great to dump 
the size and the origin of each piece of iterator tree (e.g. labels), but I 
also think that it might be useful to have "debugging" format so I couldn't 
figure out what's the best approach here.

> I thought we might be replacing this soon. Of course there's no fundamental 
> reason we can't keep both but I'm curious why this is an area of focus :-)

Yes, we are. Initially, this wasn't an area of focus: I just forgot to update 
the comment in `Iterator.h` when moving `PostingList` to a separate file and 
slightly changing the format, but then Eric had a good proposal and I thought 
that it's a good improvement. Also, even though the implementation will be 
different, the dumping format could be the same, so it's not really 
implementation-specific.


https://reviews.llvm.org/D52084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52084: [clangd] Improve PostingList iterator string representation

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

This change seems fine but...

This representation with the raw DocIDs and position dumped seems mostly useful 
for debugging buggy iterator implementations, but not really useful for 
understanding query structure and behavior.

I thought we might be replacing this soon. Of course there's no fundamental 
reason we can't keep both but I'm curious why this is an area of focus :-)


https://reviews.llvm.org/D52084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52084: [clangd] Improve PostingList iterator string representation

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 165973.
kbobyrev marked an inline comment as done.

https://reviews.llvm.org/D52084

Files:
  clang-tools-extra/clangd/index/dex/Iterator.h
  clang-tools-extra/clangd/index/dex/PostingList.cpp
  clang-tools-extra/unittests/clangd/DexTests.cpp


Index: clang-tools-extra/unittests/clangd/DexTests.cpp
===
--- clang-tools-extra/unittests/clangd/DexTests.cpp
+++ clang-tools-extra/unittests/clangd/DexTests.cpp
@@ -262,13 +262,15 @@
   const PostingList L4({0, 1, 5});
   const PostingList L5({});
 
-  EXPECT_EQ(llvm::to_string(*(L0.iterator())), "[4]");
+  EXPECT_EQ(llvm::to_string(*(L0.iterator())), "[4 ...]");
 
   auto Nested =
   createAnd(createAnd(L1.iterator(), L2.iterator()),
 createOr(L3.iterator(), L4.iterator(), L5.iterator()));
 
-  EXPECT_EQ(llvm::to_string(*Nested), "(& (| [5] [1] [END]) (& [1] [1]))");
+  EXPECT_EQ(
+  llvm::to_string(*Nested),
+  "(& (| [... 5] [... 1 ...] [END]) (& [1 ...] [1 ...]))");
 }
 
 TEST(DexIterators, Limit) {
Index: clang-tools-extra/clangd/index/dex/PostingList.cpp
===
--- clang-tools-extra/clangd/index/dex/PostingList.cpp
+++ clang-tools-extra/clangd/index/dex/PostingList.cpp
@@ -61,10 +61,14 @@
 private:
   llvm::raw_ostream (llvm::raw_ostream ) const override {
 OS << '[';
+if (Index != std::begin(Documents))
+  OS << "... ";
 if (Index != std::end(Documents))
   OS << *Index;
 else
   OS << "END";
+if (Index != std::end(Documents) && Index != std::end(Documents) - 1)
+  OS << " ...";
 OS << ']';
 return OS;
   }
Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -93,9 +93,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[... ID ...]" where ID is the element under iterator
+  /// cursor.
   friend llvm::raw_ostream <<(llvm::raw_ostream ,
const Iterator ) {
 return Iterator.dump(OS);


Index: clang-tools-extra/unittests/clangd/DexTests.cpp
===
--- clang-tools-extra/unittests/clangd/DexTests.cpp
+++ clang-tools-extra/unittests/clangd/DexTests.cpp
@@ -262,13 +262,15 @@
   const PostingList L4({0, 1, 5});
   const PostingList L5({});
 
-  EXPECT_EQ(llvm::to_string(*(L0.iterator())), "[4]");
+  EXPECT_EQ(llvm::to_string(*(L0.iterator())), "[4 ...]");
 
   auto Nested =
   createAnd(createAnd(L1.iterator(), L2.iterator()),
 createOr(L3.iterator(), L4.iterator(), L5.iterator()));
 
-  EXPECT_EQ(llvm::to_string(*Nested), "(& (| [5] [1] [END]) (& [1] [1]))");
+  EXPECT_EQ(
+  llvm::to_string(*Nested),
+  "(& (| [... 5] [... 1 ...] [END]) (& [1 ...] [1 ...]))");
 }
 
 TEST(DexIterators, Limit) {
Index: clang-tools-extra/clangd/index/dex/PostingList.cpp
===
--- clang-tools-extra/clangd/index/dex/PostingList.cpp
+++ clang-tools-extra/clangd/index/dex/PostingList.cpp
@@ -61,10 +61,14 @@
 private:
   llvm::raw_ostream (llvm::raw_ostream ) const override {
 OS << '[';
+if (Index != std::begin(Documents))
+  OS << "... ";
 if (Index != std::end(Documents))
   OS << *Index;
 else
   OS << "END";
+if (Index != std::end(Documents) && Index != std::end(Documents) - 1)
+  OS << " ...";
 OS << ']';
 return OS;
   }
Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -93,9 +93,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[... ID ...]" where ID is the element under iterator
+  /// cursor.
   friend llvm::raw_ostream <<(llvm::raw_ostream ,
const Iterator ) {
 return Iterator.dump(OS);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits